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

NickAkhmetov/HMP-159 Upgrade vitessce-python dependency #81

Merged
merged 27 commits into from
Aug 14, 2023

Conversation

NickAkhmetov
Copy link
Collaborator

@NickAkhmetov NickAkhmetov commented Jul 18, 2023

This PR updates the vitessce-python dependency from 1.0.9 to 3.0.5; this was done as part of troubleshooting HMP-296. There are still failing tests etc, which will be updated accordingly once the core issue in 296 is resolved.

@NickAkhmetov
Copy link
Collaborator Author

Current state:

  • All the assays on the portal which have vitessce hints display identical to prod
  • Potential issue: I can't get the obsLabelsType coordination that's supposed to take the place of the factors to work as expected right now. When the related code is uncommented and the duplicate obs sets are removed, the tooltips show up with blank values for all marker genes, regardless of whether the coordination value is prefixed with obs. Continuing to investigate this a bit further.

Without obs/:
image

With obs/:
image

Sample vitessce config with the marker genes issue

{
  "coordinationSpace": {
    "dataset": {
      "A": "A"
    },
    "embeddingType": {
      "A": "UMAP"
    },
    "obsLabelsType": {
      "A": "marker_gene_0",
      "B": "marker_gene_1",
      "C": "marker_gene_2",
      "D": "marker_gene_3",
      "E": "marker_gene_4"
    }
  },
  "datasets": [
    {
      "files": [
        {
          "fileType": "anndata.zarr",
          "options": {
            "featureLabels": {
              "path": "var/hugo_symbol"
            },
            "obsEmbedding": [
              {
                "dims": [
                  0,
                  1
                ],
                "embeddingType": "UMAP",
                "path": "obsm/X_umap"
              }
            ],
            "obsFeatureMatrix": {
              "initialFeatureFilterPath": "var/marker_genes_for_heatmap",
              "path": "X"
            },
            "obsSets": [
              {
                "name": "Leiden",
                "path": "obs/leiden"
              }
            ]
          },
          "url": "https://assets.hubmapconsortium.org/6c57274e7a40413dc042ec32442a228b/hubmap_ui/anndata-zarr/secondary_analysis.zarr"
        }
      ],
      "name": "6c57274e7a40413dc042ec32442a228b",
      "uid": "A"
    }
  ],
  "description": "",
  "initStrategy": "auto",
  "layout": [
    {
      "component": "scatterplot",
      "coordinationScopes": {
        "dataset": "A",
        "embeddingType": "A",
        "obsLabelsType": [
          "A",
          "B",
          "C",
          "D",
          "E"
        ]
      },
      "h": 6,
      "w": 9,
      "x": 0,
      "y": 0
    },
    {
      "component": "obsSets",
      "coordinationScopes": {
        "dataset": "A",
        "obsLabelsType": [
          "A",
          "B",
          "C",
          "D",
          "E"
        ]
      },
      "h": 3,
      "w": 3,
      "x": 9,
      "y": 0
    },
    {
      "component": "featureList",
      "coordinationScopes": {
        "dataset": "A",
        "obsLabelsType": [
          "A",
          "B",
          "C",
          "D",
          "E"
        ]
      },
      "h": 3,
      "w": 3,
      "x": 9,
      "y": 4
    },
    {
      "component": "obsSetFeatureValueDistribution",
      "coordinationScopes": {
        "dataset": "A",
        "obsLabelsType": [
          "A",
          "B",
          "C",
          "D",
          "E"
        ]
      },
      "h": 4,
      "w": 5,
      "x": 7,
      "y": 6
    },
    {
      "component": "heatmap",
      "coordinationScopes": {
        "dataset": "A",
        "obsLabelsType": [
          "A",
          "B",
          "C",
          "D",
          "E"
        ]
      },
      "h": 4,
      "w": 7,
      "x": 0,
      "y": 6
    }
  ],
  "name": "6c57274e7a40413dc042ec32442a228b",
  "version": "1.0.15"
}

@NickAkhmetov
Copy link
Collaborator Author

I put together a local draft release of vitessce-python 3.0.7 using some of my pending PR's:

I verified these changes work as expected locally; CI fails for this branch because 3.0.7 is not yet a released Vitessce version. Once those three PR's are merged and a new release is made, I will update the test fixtures again and open this PR for review.

I've also preemptively pushed a portal-visualization package version bump as part of this PR given the change in dependency version.

@NickAkhmetov
Copy link
Collaborator Author

  • Vitessce 3.0.7 is released
  • Fixtures are updated
  • Tests pass
  • Smoke test of portal visualizations loaded as expected

Ergo, I'm opening this for review.

@NickAkhmetov NickAkhmetov marked this pull request as ready for review August 11, 2023 18:33
@@ -0,0 +1,3 @@
{
"editor.rulers": [100]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this .vscode config as a guide to avoid long comment lines that broke lint

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.

Not sure if my review is valuable here, but looks good to me!

@@ -50,7 +62,7 @@ def get_conf_cells(self, marker=None):
if f'{zarr_path}/.zgroup' not in file_paths_found:
message = f'RNA-seq assay with uuid {self._uuid} has no .zarr store at {zarr_path}'
raise FileNotFoundError(message)
vc = VitessceConfig(name=self._uuid)
vc = VitessceConfig(name=self._uuid, schema_version='1.0.15')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we define schema_version somewhere for reuse?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great call! I've added this as a private field in the BaseBuilder which allows for overrides to be done (if ever needed) by passing schema_version to the args of any builder.

Comment on lines 57 to 59
"coordination_values": {
"obsType": "cell",
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could it be worth introducing a util or similar for coordination_values? There is a bit of repetition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a create_coordination_values util which:

  • Returns `{ "obsType": "cell" } by default (most common case)
  • Can have a different obs_type specified by passing an arg (first positional arg or named obs_type)
  • Can have other keys specified as named args, e.g. embeddingType="UMAP"

@NickAkhmetov
Copy link
Collaborator Author

After resolving John's comments above and getting verbal approval from Mark via slack, I'm merging these changes in 🙌

@NickAkhmetov NickAkhmetov merged commit ac8e4c3 into main Aug 14, 2023
2 checks passed
@NickAkhmetov NickAkhmetov deleted the nickakhmetov/hmp-159-upgrade-vitessce-python branch August 14, 2023 16:58
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