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

Allow to compose datasets without transforms or with bigwarp/NML landmarks #7395

Merged
merged 55 commits into from
Jan 16, 2024

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Oct 17, 2023

image

This PR adds the ability to compose a new dataset from existing dataset layers. There are three possibilities to do this:

  • combine datasets without any transforms
  • combine datasets with a BigWarp CSV that contains landmarks. Transforms are automatically created.
  • combine datasets with two WK NMLs that contain landmarks. Transforms are automatically created.

The user is guided through these options in a wizard. Transforms are only generated for a pair of datasets (i.e., it is not yet possible to compose from more than two datasets).

Slack discussion

URL of deployed dev instance (used for testing):

Steps to test:

  • use the wizard to test the individual options
  • no transforms:
    • simply pick one or more datasets
    • edit the layers afterwards bit
    • create the dataset and view it
  • for the other two modes:
    • download this archive.
    • import the datasets by moving the dataset folders into your binaryData folder
    • then compose a new dataset each by providing the landmark files
    • in the CSV case, you need to pass the dataset names manually (ROI2017_wkw_tiffs_1_wkw and ROI2017_wkw_tiffs_2_wkw)

Issues:


(Please delete unneeded items, merge only when none are left open)

@philippotto philippotto self-assigned this Oct 17, 2023
@philippotto
Copy link
Member Author

  • I cannot progress further than the dataset selection. I chose no transforms in the first step and then regardless of which datasets I select, I get the message that the datasets don't exist.
    not_find_datasets

Hmm, this is weird. It works for me. Can you check the network tab and understand why the dataset can't be found? Happy to debug this together in a call 🤙

  • The dataset selection behaves a little un-intuitive: (1) When first clicking into the selection, no dataset names are shown. (2) After typing, results that match the typed string are shown. (3) After deleting the typed string, some but not all datasets are shown (I think all of the datasets that matched a typed string at some point). - (2) is good, but for (1) and (3) maybe a list of all available dataset names could be shown?

I made the component an async one to avoid that all datasets have to be fetched. Instead, the same search api is used as in the dashboard. To reduce the confusion I changed two things:

  • I adapted the placeholder to read: Type to search and select dataset(s)...
  • I added code that clears the suggestions after selecting an entry. This means that clicking into the field won't show old suggestions. Thus, the user shouldn't assume that these are all available datasets. Instead, they hopefully type again.

It's not perfect, but I can't think of a better way right now without fetching possibly thousands of datasets in this UI. I hope this is okay?

@frcroth frcroth requested a review from fm3 January 8, 2024 10:27
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Backend LGTM :)

@daniel-wer
Copy link
Member

@philippotto Thanks for addressing my feedback! The added format instructions will be very helpful to users I would imagine.

Hmm, this is weird. It works for me. Can you check the network tab and understand why the dataset can't be found? Happy to debug this together in a call 🤙

This turned out to be an sql issue of the dev deployment. I wrongly assumed I was the first to deploy the branch at the time, but it was deployed earlier and the schemaVersion became incompatible. After a re-deployment, this works well 👍

I added code that clears the suggestions after selecting an entry. This means that clicking into the field won't show old suggestions. Thus, the user shouldn't assume that these are all available datasets. Instead, they hopefully type again.
It's not perfect, but I can't think of a better way right now without fetching possibly thousands of datasets in this UI. I hope this is okay?

This mostly works well and addresses my issue, but a small nuisance now is that typing and then clicking anywhere but on a dataset entry will clear the search box. I'm not sure whether that's important, but if you see a quick fix, feel free.

One optional suggestion would be to use toggles to disable layers in the layer selection screen, because it would allow users to change their mind (going back one screen and next again, resets all values including the dataset name).

@fm3 Feel free to test as well and/or approve if you are happy with this PR :)

@philippotto
Copy link
Member Author

This mostly works well and addresses my issue, but a small nuisance now is that typing and then clicking anywhere but on a dataset entry will clear the search box. I'm not sure whether that's important, but if you see a quick fix, feel free.

Hmm, this seems to be the default behavior in the select component from antd (see doc examples). I didn't find a way to avoid this (I tried autoClearSearchValue and onClear). So, there's not much we can do for now, I'm afraid.

One optional suggestion would be to use toggles to disable layers in the layer selection screen, because it would allow users to change their mind (going back one screen and next again, resets all values including the dataset name).

Good idea! However, I'd like to postpone this to another iteration :) We will certainly get more feedback when this feature is available in production and then we can priotize.

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Works for me :)

I think the first page of the wizard is a bit cumbersome. It contains three explanations, then a radio group with three buttons that mirror those, and then a next button. Instead it could be three clickable cards, where each starts one of the workflows, and which contain the explanations. To my mind that would be a way clearer UI.

However, this does not need to block this PR

@fm3
Copy link
Member

fm3 commented Jan 16, 2024

@frcroth One more question: I see that the new case class DataLayerId used inside of ComposeRequestLayer has name and owningOrganization. Isn’t this the same as DataSourceId? Should that just be used instead? I think this thing shouldn’t be called DataLayerId as it does not actually identify a layer, but rather a dataset. Maybe another way could be found here

@philippotto
Copy link
Member Author

I think the first page of the wizard is a bit cumbersome. It contains three explanations, then a radio group with three buttons that mirror those, and then a next button. Instead it could be three clickable cards, where each starts one of the workflows, and which contain the explanations. To my mind that would be a way clearer UI.

Good point! We don't have off-the-shelf cards with a nice design yet. However, I decided to slim down the UI by directly inlining the radio buttons where the list was before. The description was somewhat redundant anyway (the next pages of the wizard explain what's needed better). I hope this is okay that way.

image

@fm3
Copy link
Member

fm3 commented Jan 16, 2024

Definitely better :) :shipit:

@philippotto
Copy link
Member Author

@frcroth One more question: I see that the new case class DataLayerId used inside of ComposeRequestLayer has name and owningOrganization. Isn’t this the same as DataSourceId? Should that just be used instead? I think this thing shouldn’t be called DataLayerId as it does not actually identify a layer, but rather a dataset. Maybe another way could be found here

Should I wait with merging this PR?

@fm3
Copy link
Member

fm3 commented Jan 16, 2024

Should I wait with merging this PR?

Can be a follow-up. I guess we can change the API later without issue, since it is not used except by our own frontend code

@philippotto philippotto enabled auto-merge (squash) January 16, 2024 12:45
@philippotto philippotto merged commit 08a8e7f into master Jan 16, 2024
2 checks passed
@philippotto philippotto deleted the compose-datasets branch January 16, 2024 13:10
@frcroth
Copy link
Member

frcroth commented Jan 17, 2024

@frcroth One more question: I see that the new case class DataLayerId used inside of ComposeRequestLayer has name and owningOrganization. Isn’t this the same as DataSourceId? Should that just be used instead? I think this thing shouldn’t be called DataLayerId as it does not actually identify a layer, but rather a dataset. Maybe another way could be found here

Discussed here: https://scm.slack.com/archives/C5AKLAV0B/p1700831539308049?thread_ts=1697184159.482769&cid=C5AKLAV0B
Yes, can be a followup

@fm3
Copy link
Member

fm3 commented Jan 17, 2024

you’re right, I now wrote #7560

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.

Import of big warp transformations
4 participants