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

[ENH] Datasets: Let the filter override domain and language #6930

Merged
merged 7 commits into from
Nov 15, 2024

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Nov 14, 2024

Issue

Adding the domain and language combo made it more difficult to load educational data sets.

Also fixes #6931.

Description of changes

Make the filter override the domain (and language) setting. The number of data sets is large, so users always filter them anyway.

The fix may also provide an alternative to 16bbfdf? @markotoplak

Includes
  • Code changes
  • Tests

@janezd
Copy link
Contributor Author

janezd commented Nov 14, 2024

@markotoplak, can you take a look at this?

A better fix

Your fix for unpublished data sets works, but it's rather a hack. The reason why you had to do it is, basically, that selected_id depends on the filter, but selected_id is a setting while filter is not. On the other hand, we don't want to add the filter to setting.

Your trick wouldn't work in this PR. I'd either have to change (= override the saved) language and/or domain combo, or add filter to settings. So I added a "filter hint", which sets the filter when necessary.

Why it still won't work

There is another problem, related to an existing inconsistency (essentially from the previous PR):

  • use the filter to override the domain/language/unpublished,
  • double-click a data set that would not be shown without the filter; the data is downloaded and output
  • clear the filter.

The data set is still output, but selected_id is set to None. The user doesn't notice anything strange, but if you save the workflow and reload it, no data set is selected (because selected_id is saved as None).

Perhaps the best fix is that selected_id overrides the filter: if a data set is selected, it is shown, no matter what any filter and domain and language and unpublished say...

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 96.07843% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@2869a55). Learn more about missing BASE report.
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #6930   +/-   ##
=========================================
  Coverage          ?   88.45%           
=========================================
  Files             ?      331           
  Lines             ?    73099           
  Branches          ?        0           
=========================================
  Hits              ?    64662           
  Misses            ?     8437           
  Partials          ?        0           

@markotoplak
Copy link
Member

At first I immediately agreed that selected_id should override the filter. But if it does, then searching for something new will always keep the selected data showing, too, which can be confusing. No idea what is good.

When I was reviewing the previous PR, I found the same issue: output != selection. So what is saved is the actual selection, not the output. It can also be problematic in other ways: you output D1 then select (single click) D2, save. In the loaded workflow, we get D2.

I think the current selected_id should represent the output, which should be decoupled from the selection (we probably do not need a setting for it).

@janezd janezd added this to the 3.38.0 milestone Nov 15, 2024
@janezd janezd force-pushed the dataset-filter-overrides-domain branch from 4e185c5 to d759a3f Compare November 15, 2024 10:57
@janezd janezd force-pushed the dataset-filter-overrides-domain branch from d759a3f to 08744a4 Compare November 15, 2024 11:26
@markotoplak markotoplak merged commit 87334be into biolab:master Nov 15, 2024
25 of 30 checks passed
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.

Datasets: Wrong data set after reloading workflow
2 participants