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

Tkakar/cat 673 create builder for seg mask #92

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tkakar
Copy link
Collaborator

@tkakar tkakar commented Sep 13, 2024

Creates the epic-builder to update the vitessce configuration of a parent dataset for the provided segmentation mask.

@tkakar
Copy link
Collaborator Author

tkakar commented Sep 13, 2024

@NickAkhmetov not sure why the tests are failing, I removed the parts for epic_builder for now, is there a special setup that is needed?

Copy link
Collaborator

@NickAkhmetov NickAkhmetov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work so far! I did spot some minor documentation phrasing suggestions and a few small improvements that stood out to me as opportunities.

I suspect we probably won't make a new release of portal-vis until the builder logic is more finalized?

def __init__(self, base_conf: ConfCells, epic_uuid) -> None:


class EPICConfBuilder(ViewConfBuilder): # pragma: no cover
Copy link
Collaborator

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 base ViewConfBuilder (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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
seg_path = (
'https://assets.hubmapconsortium.org/c9d9ab5c9ee9642b60dd351024968627/'
'ometiff-pyramids/VAN0042-RK-3-18-registered-PAS-to-postAF-registered.ome_mask.ome.tif?'
'token=AgzQXm7nvOW32vWw0EPpKonwbOqjNBzNvvW1p15855NoYglJxyfkC8rlJJWy8V6E8MeyXOwlpKdNBnHb5qnv7f8oeeG'
Copy link
Collaborator

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-L107

for obj in segmentation_objects:
dataset.add_object(obj)

# TODO: what happens if these views already exist , and if there are other views, how to place these?
Copy link
Collaborator

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.


args = parser.parse_args()
marker = args.marker
# epic_builder = args.epic_builder
epic_uuid = args.epic_uuid
# parent_uuid = args.parent_uuid # this may not be needed, as the --url provides the parent dataset json?
Copy link
Collaborator

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 in builder_factory expects parent 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

src/vis-preview.py Outdated Show resolved Hide resolved
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.

2 participants