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

Update to new raster schema and add multi-image demo #515

Merged
merged 38 commits into from
Apr 30, 2020

Conversation

manzt
Copy link
Member

@manzt manzt commented Apr 28, 2020

I'd wished that a more general solution for our CSS could be a part of this PR, but I think that it's beyond the scope of the PR and would rather have the model for adding/changing/removing image layers from a spatial component in place before tackling that.

The primary changes in this PR are:

  • 1.) Adding the new raster.json at the top level and changing Channels component to LayerController which is responsible for adding/removing/changing vivImage layers in the Spatial component.
  • 2.) RASTER_ADD events are now subscribed to in the LayerController, which can publish LAYER_ADD/LAYER_REMOVE/LAYER_CHANGE events (which are subscribed to in the SpatialSubscriber).
  • 3.) Add a new demo for the Spraggins data where both IMS data and MxIF data can be viewed together by adding image layers.
  • 4.) Update viv to v0.2.0

I've tried to add comments and be clear about things on the horizon after this PR, which include:

  • 1.) Ability to load initial "state" on load. Right now if a raster source is supplied, image layer must be added manually, you need to assign channels to get to the desired view.
  • 2.) Ability to change slider ranges for images. Linnarsson data are very much left skewed we should adjust the domain of the sliders.
  • 3.) Decide on a CSS solution. I carried over Material UI work from the viv demo, but the components don't fit as well in the Linnarsson example, and I'd like to get away from Material Grid since we can use CSS grid with styled-componets.

@manzt
Copy link
Member Author

manzt commented Apr 28, 2020

vitessce-ims-mxif

@manzt
Copy link
Member Author

manzt commented Apr 28, 2020

hmm this seems related to travis issue: storybookjs/storybook#10477

.travis.yml Outdated
@@ -2,7 +2,7 @@ sudo: false

language: node_js
node_js:
- 13
- 12
Copy link
Member Author

Choose a reason for hiding this comment

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

Temporary fix for issue with babel storybookjs/storybook#10477

@ilan-gold
Copy link
Collaborator

First and foremost, holy smokes! This is it! The overlay is gorgeous and I love how the opacity works. The controllers are gorgeous and intuitive.

A few questions and observations before I start reviewing code:

  1. Why can't we have an "initial state" i.e where perhaps we load the first n channels? I understand that the outlier here is the IMS data, but perhaps we can load channels initially only for the pyramid data?

  2. What happened to the Linnarsson slider range? I thought we had that in vitessce-data, but maybe it got lost in the sauce. I don't remember it being removed in any PR but I also can't remember it being discussed in the talk of the schema so it may have been just left out all together.

    a. If this range business is solved back to what it was, I think we should have the sliders' lower range start some fixed percentage of the range, however small, off from from 0. This is how OMERO does it (they seem to have presets but low bound is rarely on 0). I think there is a lot of noise in that lower bound.

  3. It seems like there was a regression in the Linnarsson sliders with the spacing. Also, and perhaps this is just me, the controller feels too short? Perhaps the answer is a new layout? I came up with this, which solves the later but not the former:

Screen Shot 2020-04-29 at 10 29 34 AM

I would not say any of these are blockers, though, especially if Nils has given the OK to each or any of them. On to code!

Copy link
Collaborator

@mccalluc mccalluc left a comment

Choose a reason for hiding this comment

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

Looks good, I think: Most of the comments are in the json schema. Almost all about trees and not the forest... if there's a bigger picture I should try to think about, let me know?

src/components/layer-controller/ImageAddButton.js Outdated Show resolved Hide resolved
src/components/layer-controller/LayerController.js Outdated Show resolved Hide resolved
src/components/layer-controller/LayerController.js Outdated Show resolved Hide resolved
src/schemas/raster.schema.json Show resolved Hide resolved
src/schemas/raster.schema.json Outdated Show resolved Hide resolved
"examples": ["quantitative", "nominal", "ordinal", "temporal"]
},
"values": {
"type": ["array", "null"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Less special case checking down stream if this is always an array, perhaps of zero length?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently what we decided in in vitessce-data is that null is allowed. Ilan and I have been talking about how to represent dimensions better, but I think we should allow this for the time being. vitessce/vitessce-data#90

}
}
},
"imageDimensions": {
"type": "array",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will the length typically be 2? Add a constraint for that? -- Or I may be misunderstanding the purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

image dimensions are defined as and array of {name, type, values}. So for N-dimensional images [time, z, channel, y, x] the dimensions would be of length 5?

src/schemas/raster.schema.json Outdated Show resolved Hide resolved
@keller-mark
Copy link
Member

Should the "Add Image Layer" button press also automatically add one (the first) channel? Is there a case in which you would want an image layer but not at least one channel?

Unrelated: is the goal for the Layers > molecules, cells, neighborhoods to eventually be moved into this component?

@manzt
Copy link
Member Author

manzt commented Apr 29, 2020

@ilan-gold

Why can't we have an "initial state" i.e where perhaps we load the first n channels? I understand that the outlier here is the IMS data, but perhaps we can load channels initially only for the pyramid data?

We hadn't discussed a heuristic, and I didn't think there was an "obvious" choice. Making a small PR to add layers automatically based on said heuristic should be very straight-forward to implement, but I wanted to include others in the discussion. On top of that, I think it would be great to have a declarative way to specify this (which would override any heuristic we have). Perhaps something in the raster schema, but again, I think that's beyond the scope of this PR.

What happened to the Linnarsson slider range? I thought we had that in vitessce-data, but maybe it got lost in the sauce. I don't remember it being removed in any PR but I also can't remember it being discussed in the talk of the schema so it may have been just left out all together.

Yes, this was removed in the most recent raster schema. I think mostly because when we get image data with more than 1 extra dimension, we will need some type better structure to storing these pre-calculated ranges (i.e. a simple array of ranges doesn't work, unless we also use a notion of strides and offsets). I think this is something we should also discuss in a separate PR (since the data source no longer has the information and changing would require a change there as well).

If this range business is solved back to what it was, I think we should have the sliders' lower range start some fixed percentage of the range, however small, off from from 0. This is how OMERO does it (they seem to have presets but low bound is rarely on 0). I think there is a lot of noise in that lower bound.

Totally agree. When we come up with a way of storing these ranges, we can use some sensible heuristics.

It seems like there was a regression in the Linnarsson sliders with the spacing. Also, and perhaps this is just me, the controller feels too short? Perhaps the answer is a new layout? I came up with this, which solves the later but not the former:

This feels coupled to the CSS solution. I could spend a lot of time hashing out how to make this combination of Material just right, but I'd rather come up with a standard way of how we are going to do CSS (my vote is for styled-components perhaps with material ui) and then we can refactor these to make them better. Agreed on the new layout. Could you share the config?

"image": {
"type": "object",
"additionalProperties": false,
"required": ["name", "url", "type", "metadata"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure there is an action item here, but I think metadata will be optional (might change when I integrate CODEX) since I anticipate OMEXML capturing most of that for pyramids/CODEX.

Copy link
Member Author

@manzt manzt Apr 29, 2020

Choose a reason for hiding this comment

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

Yeah definitely good to have on our radar, but I think right now the machinery in vitessce is set up expecting these metadata. In the next cycle, I think vitessce should get these data from the loaders after initialized:

const { dimensions } = loader;

instead of:

const { metadata } = raster_json;
const { dimensions } = metadata;

@ilan-gold
Copy link
Collaborator

@manzt Agreed on styled components. I'll share the config momentarily. I agree with Mark about adding one channel

@ilan-gold
Copy link
Collaborator

ilan-gold commented Apr 29, 2020

I believe this config will work for the static layout:

[
      { component: 'description',
        props: {
          description: `Linnarsson: ${linnarssonDescription}`,
        },
        x: 0, y: 0, w: 2, h: 1 },
      { component: 'layerController',
        x: 0, y: 1, w: 2, h: 4,
      },
      { component: 'status',
        x: 0, y: 5, w: 2, h: 1 },
      { component: 'spatial',
        props: {
          view: {
            zoom: -5.5,
            target: [16000, 20000, 0],
          },
        },
        x: 2, y: 0, w: 4, h: 4 },
      { component: 'scatterplot',
        props: {
          mapping: 'PCA',
          // This intentionally does not have a  "view" prop,
          // in order to have an example that uses the default.
        },
        x: 6, y: 0, w: 4, h: 2 },
      { component: 'scatterplot',
        props: {
          mapping: 't-SNE',
          view: {
            zoom: 0.75,
            target: [0, 0, 0],
          },
        },
        x: 6, y: 2, w: 4, h: 2 },
      { component: 'factors',
        x: 10, y: 0, w: 1, h: 2 },
      { component: 'genes',
        x: 11, y: 2, w: 1, h: 2 },
      { component: 'cellSets',
        props: {
          datasetId: 'linnarsson-2018',
        },
        x: 10, y: 3, w: 2, h: 2 },
      { component: 'heatmap',
        x: 2, y: 4, w: 10, h: 2 },
    ]

@manzt
Copy link
Member Author

manzt commented Apr 29, 2020

Ok, I changed the Linnarsson layout and tried to address many of the comments above. Now pressing "Add Image Layer" will automatically add a channel (and have the layer panel open by default). I think the slider ranges should be addressed in a separate PR, along with exploring styled-components. I tried to consolidate much of the Material UI style overrides

@manzt manzt changed the base branch from master to dev April 30, 2020 14:07
@manzt manzt merged commit 1dcec82 into dev Apr 30, 2020
@manzt manzt deleted the manzt/update-raster-json branch April 30, 2020 14:30
keller-mark added a commit that referenced this pull request May 5, 2020
* Change rectangle selection to dragging (#520)

* Update to new raster schema and add multi-image demo (#515)

* Add Material UI Icons

* Change `Channels` component to `LayerController`

* Use raster schema v0.0.1

* Migrate to functional subscribers and allow for multiple layers

* Reduce size of PubSub `LAYER_CHANGE` events

* Memoize derived data in Subscribers

* Change Linnarsson static layout

* Add a higlass component (#523)

* Initial higlass commit

* Update styles, use onReady function

* Update (#482)

* Change height calculation

* Changelog

* Lazy load higlass component

* Lint

* Minor update

* Merge

* Clean up higlass integration, update styles, move higlass viewconfig into vitessce viewconfig

* Whitespace

* Styles

* Comments

* Remove bootstrap

* Remove more bootstrap

* Update src/components/higlass/HiGlassSubscriber.js

Co-authored-by: Trevor Manz <[email protected]>

* Lint

* Clean up css

* Clean up css again

* Remove weird line height value

* Figure out react.lazy

Co-authored-by: Sehi L'Yi <[email protected]>
Co-authored-by: Trevor Manz <[email protected]>

* 0.1.1

* Revert "0.1.1"

This reverts commit 88eb6a0.

* Clear rectangle or lasso selection on click (#530)

* Handle clicks during rectangle and lasso

* Use constants for edit types rather than strings/nulls

* viv 0.2.2 (#531)

* Upgrade viv to 0.2.2

* Changelog

* Changelog version

* 0.1.1

* Changelog

* Changelog

Co-authored-by: Trevor Manz <[email protected]>
Co-authored-by: Sehi L'Yi <[email protected]>
keller-mark added a commit that referenced this pull request May 5, 2020
* Change rectangle selection to dragging (#520)

* Update to new raster schema and add multi-image demo (#515)

* Add Material UI Icons

* Change `Channels` component to `LayerController`

* Use raster schema v0.0.1

* Migrate to functional subscribers and allow for multiple layers

* Reduce size of PubSub `LAYER_CHANGE` events

* Memoize derived data in Subscribers

* Change Linnarsson static layout

* Add a higlass component (#523)

* Initial higlass commit

* Update styles, use onReady function

* Update (#482)

* Change height calculation

* Changelog

* Lazy load higlass component

* Lint

* Minor update

* Merge

* Clean up higlass integration, update styles, move higlass viewconfig into vitessce viewconfig

* Whitespace

* Styles

* Comments

* Remove bootstrap

* Remove more bootstrap

* Update src/components/higlass/HiGlassSubscriber.js

Co-authored-by: Trevor Manz <[email protected]>

* Lint

* Clean up css

* Clean up css again

* Remove weird line height value

* Figure out react.lazy

Co-authored-by: Sehi L'Yi <[email protected]>
Co-authored-by: Trevor Manz <[email protected]>

* 0.1.1

* Revert "0.1.1"

This reverts commit 88eb6a0.

* Clear rectangle or lasso selection on click (#530)

* Handle clicks during rectangle and lasso

* Use constants for edit types rather than strings/nulls

* viv 0.2.2 (#531)

* Upgrade viv to 0.2.2

* Changelog

* Changelog version

* 0.1.1

* Changelog

* Changelog

* Fix horizontal scroll bug (#534)

* Fix horizontal scroll bug

* Changelog

* Update CHANGELOG.md

Co-authored-by: Trevor Manz <[email protected]>
Co-authored-by: Sehi L'Yi <[email protected]>
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.

4 participants