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.

Sign up for GitHub

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

Jump to bottom

NickAkhmetov/Improve vis-lifting logic, add visium support #84

Merged
merged 16 commits into from
Mar 1, 2024
Merged

NickAkhmetov/Improve vis-lifting logic, add visium support #84

merged 16 commits into from
Mar 1, 2024

Conversation

NickAkhmetov
Copy link
Collaborator

@NickAkhmetov NickAkhmetov commented Feb 20, 2024 •
edited by jira bot
Loading

This PR:

  • Adds a builder for the multiomic visium datasets and adjusts the builder factory to include this logic. CAT-159
  • Prevents re-iterations through hints list by converting to set and making includes logic more self-explanatory
  • Removes builder factory dependencies on provenance DAG
    • DAG iteration is still used as a condition to determine whether to check for annotations; this step could be removed, however, since this should realistically be accounted for by the check for is_annotated in the annotation metadata.
  • Revised the vis-lifting approach to allow providing the parent entity as an additional argument instead
    • This decreases the number of requests made for this since the previous approach was to fetch the assaytype information for each immediate ancestor of a dataset
  • Provides sample entity ID for JSON-based CODEX dataset in a comment to ease testing
  • Adds predicted CLID to tooltips for datasets that have them CAT-1

@NickAkhmetov
Copy link
Collaborator Author

NickAkhmetov commented Feb 26, 2024 •
edited
Loading

The test_entity_to_vitessce_conf tests now pass.

Remaining todos:

  • Fix test_entity_to_error
  • Add test_entity_to_vitessce_conf test case for Visium builder
  • Add coverage for predicted_CLID-related code

The predicted-CLID related tests are inconsistent - the tests passed locally but failed in CI. Revising my approach to simply include predicted CLID for all cases where predicted label exists, updating the mocks tomorrow morning.

@NickAkhmetov NickAkhmetov mentioned this pull request Feb 27, 2024
Closed
@NickAkhmetov NickAkhmetov marked this pull request as ready for review February 28, 2024 17:53
john-conroy
john-conroy reviewed Feb 29, 2024
View reviewed changes
Copy link
Collaborator

@john-conroy john-conroy left a comment

Choose a reason for hiding this comment

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

Will parent be added to the entity in portal-ui code after vis-lifting?

@NickAkhmetov
Copy link
Collaborator Author

Will parent be added to the entity in portal-ui code after vis-lifting?

If there are any image pyramid descendants found for the current entity, get_vitessce_conf_cells recurses with the found descendant as the entity and the current entity in the parent field to assemble the conf.

@john-conroy
Copy link
Collaborator

Gotcha, parent is newly defined in the upcoming portal-ui PR https://github.com/hubmapconsortium/portal-ui/pull/3387/files.

keller-mark
keller-mark reviewed Feb 29, 2024
View reviewed changes
Copy link
Member

@keller-mark keller-mark left a comment

Choose a reason for hiding this comment

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

Awesome Nick! I left comments mostly regarding the dataset initialization and the view linking.

src/portal_visualization/builders/anndata_builders.py Outdated Show resolved Hide resolved
src/portal_visualization/builders/anndata_builders.py Outdated
})

# Indicate obs type for all views
vc.link_views(spot_views, ['obsType'], ['spot'])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
vc.link_views(spot_views, ['obsType'], ['spot'])
vc.link_views(all_views, ['obsType'], ['spot'])

src/portal_visualization/builders/anndata_builders.py Outdated Show resolved Hide resolved
src/portal_visualization/builders/anndata_builders.py Outdated Show resolved Hide resolved
src/portal_visualization/builders/anndata_builders.py Outdated Show resolved Hide resolved
src/portal_visualization/builders/anndata_builders.py Outdated Show resolved Hide resolved
src/portal_visualization/builders/anndata_builders.py Outdated Show resolved Hide resolved
src/portal_visualization/builders/anndata_builders.py Outdated
obs_set_names=self._obs_set_names,
obs_labels_names=self._obs_labels_names,
obs_labels_paths=self._obs_labels_paths,
obs_locations_path="obsm/X_spatial",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
obs_locations_path="obsm/X_spatial",
obs_spots_path="obsm/X_spatial",

src/portal_visualization/builders/anndata_builders.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@NickAkhmetov NickAkhmetov requested a review from keller-mark March 1, 2024 18:06
@NickAkhmetov NickAkhmetov merged commit 085911d into main Mar 1, 2024
3 checks passed
@NickAkhmetov NickAkhmetov deleted the nickakhmetov/visium-and-vislift-revisions branch March 1, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@john-conroy john-conroy john-conroy left review comments

@keller-mark keller-mark keller-mark approved these changes

@lchoy lchoy Awaiting requested review from lchoy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NickAkhmetov @john-conroy @keller-mark