Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New feature that save mask as geojson format #306

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nanli-emory
Copy link
Collaborator

Add a --geojson option into HistoQC. The default value of geojson is False. Test the result and import intp Qupath.
This feature is associate with issue #298

Screenshot from 2024-06-11 17-38-07

@nanli-emory
Copy link
Collaborator Author

Hi @jacksonjacobs1, Please review it.

@choosehappy
Copy link
Owner

not quite, i think there was a miscommunication here. the module should be in the config file, and should be dynamic, such that the user can add it at any point during the pipeline, or even multiple times, with a tag specifying the mask to be written out. coding wise should be similar to the classification pen marking work

@nanli-emory
Copy link
Collaborator Author

Hi @choosehappy and @jacksonjacobs1,

I created a new function called saveMask2Geojson in SaveModule. Users can export a mask in geojson format dynamically.
I modified and tested by using the histoqc/config/config_clinical.ini config file for geojson mask.
Please review and provide some feedback.

@choosehappy
Copy link
Owner

yea, much closer!

1 point left:

i think " s["img_mask_use"]) " should actually be accepted as a variable with the name of the mask to save --- "img_mask_use" can and should be the default, but if someone is interested in saving e.g., the pen detector or blur mask they should be able to as well

do you see what I mean?

@nanli-emory
Copy link
Collaborator Author

Hi @choosehappy and @jacksonjacobs1,

I added an option called 'mask_name' to let the user point out what mask to save.

@choosehappy
Copy link
Owner

great, looks good to me. no remaining points on my side- will let @jacksonjacobs1 code review and comment as needed, if not merge

@choosehappy
Copy link
Owner

btw, i quick note - we can as well easily read/write gzip compressed json (an example is in my blog). normally compressed json is about 33% the size of the uncompressed. i suspect the ones that we have here are small enough that we don't need to worry about it, and its "nicer" to be able to easily see the json, but its something to keep in mind in case we have more "serious" file sizes

@CielAl
Copy link
Contributor

CielAl commented Jun 20, 2024

yea, much closer!

1 point left:

i think " s["img_mask_use"]) " should actually be accepted as a variable with the name of the mask to save --- "img_mask_use" can and should be the default, but if someone is interested in saving e.g., the pen detector or blur mask they should be able to as well

do you see what I mean?

Just in case, the current implementation use measures.find_contour, which typically finds both the inner and outer rings. In that case, your geojson output will consider the inner and outer ring as individual polygons, if I understand it correctly.

This might be relevant:
https://stackoverflow.com/questions/72040574/how-to-determine-between-inner-and-outer-contour-with-python-opencv

@jacksonjacobs1
Copy link
Collaborator

Agree with @CielAl

The geojson template will need to be updated to the multipolygon specification:
https://stevage.github.io/geojson-spec/#section-3.1.7

https://stackoverflow.com/questions/43645172/geojson-multipolygon-with-multiple-holes

I would also recommend testing whether qupath correctly renders multi-polygons imported from geojson.

@nanli-emory
Copy link
Collaborator Author

Hi @jacksonjacobs1, completed correctly showing the inner and outer rings as individual polygons. Please review the code. Thanks.
Screenshot from 2024-11-15 11-27-54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants