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 data loaders and migrate to new zarr #81

Merged
merged 21 commits into from
Feb 20, 2020
Merged

Update data loaders and migrate to new zarr #81

merged 21 commits into from
Feb 20, 2020

Conversation

manzt
Copy link
Member

@manzt manzt commented Feb 18, 2020

Ok, so a few things here. I think all logic regarding getting and setting tiles needs to be confined to data_utils/. Optimally all the metadata regarding imageWidth, imageHeight, minZoom, tileSize, and connections can be resolved in initZarr & initTiff. Right now we have no "standards" with the metadata, but this allows us to consolidate and resolve hardcoded logic in the same place. I like this direction for two reasons:

  • The configs for the source can be very minimal, simply urls and channel names.
  • It releases the need for the connections object returned init<Loader> to be the same for different types of data. The init<Datatype> function thus controls exactly what load<Datatype> receives. Therefore, if someone where inclined to write a custom loader for another tiled datatype, they could re-implement the logic and create a plugin. As long as someone has a "connection" object that they can query with x, y, z tile coordinates, they can return an array of TypedArray channels to be rendered.

Copy link
Contributor

@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.

  • Some of the eslint-disables seem vestigial? Particularly if something is turned off at the top of a file, we should explain why. (But preferably, don't ignore rules.)
  • Would it be worth adding a pythonish range util?
  • Can you file an issue describing how you'd like this to be more OO?

Beyond that, just housekeeping and stylistic comments.

demo/src/source-info.js Show resolved Hide resolved
demo/src/source-info.js Outdated Show resolved Hide resolved
Comment on lines 157 to 158
sliderValues,
colorValues,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this is a general thing we should discuss tomorrow, but as soon as the order of these goes out of wack (i.e the keys of sliderValues does not match the order of the keys of colorValues), the sliders correspond to the wrong channels. I think we need to enforce a sort call somewhere or something (maybe when the MicroscopyViewerLayer initializes): https://stackoverflow.com/questions/5467129/sort-javascript-object-by-key. At the minimum, I think we should understand why this works the way it does (there is in fact a sort call in MicroscopyViewerLayerBase).

Copy link
Member Author

Choose a reason for hiding this comment

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

What ever order the keys are provided in the config is what is respected in the rest of the application, to my knowledge.

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to look at colorValues and slider values, but the logic in the tiff_utils was necessary because we can iterate over the object as its provided.

@manzt
Copy link
Member Author

manzt commented Feb 19, 2020

Apologies for the mess with eslint-disables and leaving in the big commented block. Thanks for looking this over and for the suggestions!

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

Resolve changelog conflict and this looks good!

@manzt
Copy link
Member Author

manzt commented Feb 19, 2020

@ilan-gold hopefully this is logic you are happy with for colors. It still feels like an antipattern to see this:

const colorOptions = [
  [255, 0, 0],
  [0, 255, 0],
  [0, 0, 255],
  [255, 128, 0]
];

// converting to
const coloVals = {
  "channel0": [255, 0, 0],
  "channel1": [0, 255, 0],
  "channel2": [0, 0, 255],
  "channel3": [255, 128, 0]
}

// back to in microscopy-layer-base.js
const colorOptions = [
  [1, 0, 0],
  [0, 1, 0],
  [0, 0, 1],
  [1, .5, 0]
];

But I need to do some more thinking (and I don't think it's that important). I'd rather the Viewer layer be agnostic to the channel names somehow.

@ilan-gold
Copy link
Collaborator

Looks like changelog needs be updated but otherwise this looks good.

@mccalluc
Copy link
Contributor

(Don't block on me, but happy to re-review, if that would be useful.)

CHANGELOG.md Outdated Show resolved Hide resolved
@manzt manzt merged commit b8f1b8b into master Feb 20, 2020
@manzt manzt deleted the manzt/zarr branch February 20, 2020 15:44
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.

None yet

3 participants