Mccalluc/consistent arrays #66

mccalluc · 2022-10-13T02:01:12Z

I've made lots of comments below. There are breaking changes here which will require work in portal-ui, but it's in the direction of greater consistency:

We always return an array now, instead of an array of viewconfs for some assays and plain viewconf for others. (This includes the Null case: portal should check len instead of is None.)
The array is an array of ViteesceConfig objects: portal-ui will need to call .to_dict(): The idea is just to stay with a higher level object as long as possible, and leave it to the downstream code to pull out what it needs.

mccalluc · 2022-10-13T02:08:38Z

VERSION.txt

@@ -1 +1 @@
-0.0.6
+0.1.0


This is a breaking change to the interface, so incrementing the minor version number seems appropriate.

mccalluc · 2022-10-13T02:15:35Z

src/portal_visualization/builders/anndata_builders.py

-    def __init__(self, entity, groups_token, assets_endpoint, **kwargs):
-        super().__init__(entity, groups_token, assets_endpoint, **kwargs)
+    def __init__(self, entity, groups_token, assets_endpoint):
+        super().__init__(entity, groups_token, assets_endpoint)


The **kwargs has masked bugs: We thought we were using the right kwarg, but there was a typo, so the code assumed no value was supplied and it continued on without complaint.

mccalluc · 2022-10-13T02:17:15Z

src/portal_visualization/builders/anndata_builders.py

        # Spatially resolved RNA-seq assays require some special handling,
        # and others do not.
        self._is_spatial = False
        self._scatterplot_w = 9

-    def get_conf_cells(self, marker=None):
+    def get_configs(self, marker=None):


The Builder subclasses now define get_configs instead of get_conf_cells, which is now defined only on the base class. The idea is to factor up behavior which is shared between subclasses.

mccalluc · 2022-10-13T02:19:19Z

src/portal_visualization/builders/anndata_builders.py

@@ -77,7 +76,7 @@ def get_conf_cells(self, marker=None):
        ))

        vc = self._setup_anndata_view_config(vc, dataset, marker)
-        return get_conf_cells(vc)
+        return [vc]


See above: This method is now only responsible for returning confs

Big change: Every class now returns a list of viewconfs. For most, it's only one element, but consistency in return types is a good thing.

mccalluc · 2022-10-13T02:20:03Z

src/portal_visualization/builders/base_builders.py

+    return [
+        nbformat.v4.new_code_cell(f'from vitessce import {", ".join(imports)}'),
+        nbformat.v4.new_code_cell(f'conf = {conf_expression}\nconf.widget()'),
+    ]


Both of these are moved from utils, since this is the only code that needs them.

mccalluc · 2022-10-13T02:23:03Z

src/portal_visualization/builders/base_builders.py

-    def get_conf_cells(self, **kwargs):
-        return ConfCells(None, None)
+    def get_configs_cells(self, marker=None):
+        return ConfigsCells([], [])


For consistency, even for the Null case we return lists. (One possibility for the cells would be to generate a message that there is no visualization... Sort of depends on the direction taken by workspaces.)

mccalluc · 2022-10-13T02:24:55Z

src/portal_visualization/builders/base_builders.py

+        kwargs = {'marker': marker} if marker is not None else {}
+        configs = self.get_configs(**kwargs)
+        cells = _get_cells_from_conf_list(configs)
+        return ConfigsCells(configs, cells)


This is the new base class method that constructs the list of cells and returns a tuple.

mccalluc · 2022-10-13T02:25:37Z

src/portal_visualization/builders/base_builders.py

-    def get_conf_cells(self, **kwargs):
-        pass
+    def get_configs(self):
+        raise NotImplementedError()


Shouldn't be called... if it were, we want an error.

mccalluc · 2022-10-13T02:27:53Z

src/portal_visualization/builders/imaging_builders.py

@@ -93,7 +93,7 @@ def get_conf_cells(self, **kwargs):
        conf = vc.to_dict()
        # Don't want to render all layers
        del conf["datasets"][0]["files"][0]["options"]["renderLayers"]
-        return get_conf_cells(conf)
+        return [VitessceConfig.from_dict(conf)]


Another change for the sake of consistency: get_configs will now always return a list of configuration objects, instead of returning dicts for some classes, objects for others, and lists in a couple cases.

mccalluc · 2022-10-13T02:29:08Z

src/portal_visualization/builders/imaging_builders.py

@@ -159,13 +159,11 @@ def get_conf_cells(self, **kwargs):
            conf = vc.to_dict()
            # Don't want to render all layers
            del conf["datasets"][0]["files"][0]["options"]["renderLayers"]
-            confs.append(conf)
-        return get_conf_cells(confs)
+            confs.append(VitessceConfig.from_dict(conf))


Ditto: We're always objects now.

mccalluc · 2022-10-13T02:30:08Z

src/portal_visualization/builders/sprm_builders.py

-        self._imaging_path_regex = kwargs["imaging_path"]
+        self._base_name = base_name
+        self._image_name = base_name
+        self._imaging_path_regex = imaging_path


Mentioned above: Make kwargs explicit to avoid typo bugs.

mccalluc · 2022-10-13T02:32:16Z

test/good-fixtures/RNASeqAnnDataZarrViewConfBuilder/fake-asct-http200-published-conf.json

-  "coordinationSpace": {
-    "dataset": {
-      "A": "A"
+[


We consistently return a list now. (Hide whitespace on the diff to make review easier.)

mccalluc added 12 commits October 3, 2022 14:30

Fix unstitched vis

3f7874c

version bump

4d99793

fix two fixtures: add []

f0c0ebf

--index

ba166ef

update docs in readme

a21ef87

Merge branch 'main' into mccalluc/consistent-arrays

0f74cd4

Checkpoint: lots of refactoring; one test passes

b3f0232

tests pass!

d72b6ec

CI should pass

7d42b8b

Move utils to base_builders that are only used there

9dbe37a

Make constructor kwargs explicit

f990839

no kwargs blobs on get_configs, either

aafa5c5

mccalluc mentioned this pull request Oct 13, 2022

Add --index #65

Closed

mccalluc commented Oct 13, 2022

View reviewed changes

mccalluc requested a review from john-conroy October 13, 2022 02:39

mccalluc mentioned this pull request Oct 13, 2022

Mccalluc/consistent arrays hubmapconsortium/portal-ui#2965

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mccalluc/consistent arrays #66

Mccalluc/consistent arrays #66

mccalluc commented Oct 13, 2022 •

edited

Loading

mccalluc Oct 13, 2022

mccalluc Oct 13, 2022

mccalluc Oct 13, 2022

mccalluc Oct 13, 2022

mccalluc Oct 13, 2022 •

edited

Loading

mccalluc Oct 13, 2022

mccalluc Oct 13, 2022

mccalluc Oct 13, 2022

mccalluc Oct 13, 2022

mccalluc Oct 13, 2022

mccalluc Oct 13, 2022

mccalluc Oct 13, 2022

		@@ -1 +1 @@
		0.0.6
		0.1.0

Mccalluc/consistent arrays #66

Are you sure you want to change the base?

Mccalluc/consistent arrays #66

Conversation

mccalluc commented Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccalluc Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccalluc commented Oct 13, 2022 •

edited

Loading

mccalluc Oct 13, 2022 •

edited

Loading