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

List Currently Available Mappings #3720

Merged
merged 17 commits into from
Feb 12, 2019
Merged

List Currently Available Mappings #3720

merged 17 commits into from
Feb 12, 2019

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Feb 4, 2019

URL of deployed dev instance (used for testing):

Steps to test:

  • you need a dataset with mappings
  • back end test (probably covered by front end testing): check http://localhost:9000/data/datasets/Connectomics_Department/ROI2017_wkw/layers/segmentation/mappings?token=secretScmBoyToken
  • front end test:
    • open a dataset with mappings (view or skeleton mode)
    • enable mappings in the segmentation tab and select a mapping
    • everything should work as you expect

Screenshot & Gif

image

mapping-ui

Issues:


@philippotto
Copy link
Member

I added the corresponding UI for mappings in this PR. Should be ready to review from my point of view.

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Awesome, I didn't think I'd live to see this day - a UI for mappings. 😁
Not having to have the browser console open to activate mappings will allow for bigger mappings as well (more available memory)!
I love the progress updates, maybe we can free up the main thread a little bit during mapping creation to avoid the hefty lag, but that is low prio and for another PR :)

I think we can get rid of the global mappings object, now, right? (setupGlobalMappingsObject and related code)

app/assets/javascripts/admin/api_flow_types.js Outdated Show resolved Hide resolved
Store.dispatch(setMappingAction(_.clone(mapping), options.colors, options.hideUnmappedIds));
Store.dispatch(
setMappingAction(
"<code provided>",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change to <user provided mapping> or <custom mapping> - I did not understand what was meant here at first.

return;
}
this.setState({ isRefreshingMappingList: true });
const layerName = "segmentation";
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be this.props.segmentationLayer.name?

Copy link
Member

Choose a reason for hiding this comment

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

My hope was that this will allow mappings even when there is a new volume tracing layer, however, this doesn't work, since the actual mappings are downloaded via an url which is defined somewhere also (and again with the segmentation layer's name). I could change it there too, but in general, this is brittle, since a segmentation layer can have different names on disk, as well. So, I went back to using the actual layer name here. It's still an improvement to before.

We should rethink how we want to handle the disk segmentation layer VS volume tracing layer in the future.


export type ProgressCallback = (isDone: boolean, progressState: string) => Promise<void>;

export default function createProgressCallback(options: {
Copy link
Member

Choose a reason for hiding this comment

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

Nice! We can probably re-use this in the future 🥇

@philippotto
Copy link
Member

@fm3 Can you verify that the checkboxes are set appropriately? :) Not sure whether a datastore update is needed?

@fm3 fm3 merged commit 33c485c into master Feb 12, 2019
@fm3 fm3 deleted the list-mappings branch February 12, 2019 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Available mappings list should be up to date Allow to select a mapping via UI
3 participants