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

histogram route #4014

Merged
merged 18 commits into from
May 16, 2019
Merged

histogram route #4014

merged 18 commits into from
May 16, 2019

Conversation

youri-k
Copy link
Contributor

@youri-k youri-k commented Apr 12, 2019

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • request [...]/data/datasets/Connectomics_Department/ROI2017_wkw/layers/color/histogram

Issues:


  • Adapted wk-connect if datastore API changes ?
  • Needs datastore update after deployment
  • Ready for review

@youri-k youri-k requested a review from fm3 April 12, 2019 09:56
@youri-k youri-k self-assigned this Apr 12, 2019
@youri-k youri-k changed the title [WIP] histogram route histogram route Apr 18, 2019
@youri-k youri-k marked this pull request as ready for review April 18, 2019 16:20
@fm3
Copy link
Member

fm3 commented Apr 23, 2019

This looks promising! I spoke with philipp and we came up with some change requests:

  • rather than computing this for all layers at once, please have the layer name be a request parameter (like it is already shown in the steps to test list above). This will also lead to no histograms for segmentation layers (which don’t make much sense semantically, especially if sampled)
  • please skip zero-values (they are likely to be caused by data being sampled outside of the real bounding box, rather than actual black tissue)
  • for uint24 layers, please compute the average of the three uint8 values that are encoded for each voxel.

@daniel-wer
Copy link
Member

As discussed, it would be really helpful for #3506, if this route would also support float layers. :)

@jstriebel jstriebel self-requested a review May 9, 2019 13:24
Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

The code LGTM, @fm3's suggestions seem to be implemented. I did not test this one, but should be fine IMO.

@jstriebel
Copy link
Contributor

PS: Please add this to scalableminds/webknossos-connect#22 with a link to this PR.

@youri-k youri-k merged commit 2c44816 into master May 16, 2019
@fm3 fm3 deleted the histogram-backend-route branch May 16, 2019 08: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.

Add backend route to fetch sampled histogram data for each color layer
4 participants