Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Tkakar/cat 673 create builder for seg mask #92
Tkakar/cat 673 create builder for seg mask #92
Changes from 7 commits
fe742a2
e25f2f0
52293d3
76e9552
5092db8
169bba1
efda4a3
2331472
f558c3f
5e82cd4
bcd35f2
f5b7022
d901d92
1a77f16
8b072e8
4823e6a
d7aef66
79ad4f8
99f56af
89893c7
0ff5e07
dda6321
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if we wanted the
EPICConfBuilder
to extend the baseViewConfBuilder
(i.e. in case there are any additional EPIC-specific patterns that become apparent) but I think it should be fine; I think the extra_epic_uuid
logic you included in the base builder takes care of my initial concerns.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitGuardian nagged me about including the token in the previously committed code, even though they expire quickly 😆
I believe we can access the token via
self._groups_token
like the superclass does now that we're extending it: https://github.com/hubmapconsortium/portal-visualization/blob/main/src/portal_visualization/builders/base_builders.py#L106-L107There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, this is definitely important to determine! While we may be able to handle the more basic image-only assays like Histology/PAS microscopy by appending to existing configurations, given that we anticipate there will be segmentation masks for various assays and that we'll have a few different base confs as a result, my understanding is that we may need to re-set up views and coordinations from scratch for those. We should discuss with @keller-mark to confirm/see if he has any alternative suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
parent_uuid
is necessary for visualizing image pyramid support datasets, since the logic inbuilder_factory
expectsparent is not None
for those cases and performs an assaytype lookup on the parent to determine which specific image pyramid builder to use (to handle evolution of how image pyramids were handled over time): https://github.com/hubmapconsortium/portal-visualization/blob/main/src/portal_visualization/builder_factory.py#L75-L76