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

add genomics filter #23

Merged
merged 30 commits into from
Aug 13, 2024
Merged

add genomics filter #23

merged 30 commits into from
Aug 13, 2024

Conversation

gcroci2
Copy link
Contributor

@gcroci2 gcroci2 commented Jul 29, 2024

Main changes:

  • All the layouts elements have been moved to layout.py
  • All the callbacks have been moved to callbacks.py
  • Everything contained in the genomics -> metabolomics tab is disabled if no file is uploaded
  • The genomics filter has been added as a mantine accordion component with GCF id and BGC class filters
    • When a filter is added, the other filters and values stay. When a dropdown menu value change, the corresponding input text is cleared.
  • Opened issues Improve dark mode #25 and Improve the filter's options displaying #26 for further improvements
  • Updated tests and added a unit test for add_block. Unit testing the other callbacks is possible of course, but I'm not sure whether is worth it to do it at this stage.

@gcroci2 gcroci2 marked this pull request as draft July 29, 2024 13:20
@gcroci2 gcroci2 self-assigned this Jul 30, 2024
@gcroci2 gcroci2 marked this pull request as ready for review July 30, 2024 13:45
@gcroci2 gcroci2 linked an issue Jul 30, 2024 that may be closed by this pull request
Copy link
Member

@CunliangGeng CunliangGeng left a comment

Choose a reason for hiding this comment

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

The following points of the original design is not fully implemented (see figure). I think they are important and would like to know your thoughts on the change of the design.

  1. Show a folding sign on the "Genomics filter"
  2. Add a new filter when necessary with the sign "+"; otherwise do not show that filter at all. And when folding the filter, it shows a list of the filter name for users to choose. Here is an example of the effect.

image

@gcroci2
Copy link
Contributor Author

gcroci2 commented Jul 31, 2024

  1. Show a folding sign on the "Genomics filter"

The component I used here is a dcc.Button, which does not have any folding sign. What we wanted here was something like the arrow in the dropdown menu (see the basic example here), that changes direction when clicked. I didn't find anything similar for a button component. I think it would be possible to add a static arrow on the right side of the button to mimic the mockup, but then it wouldn't move and I am not sure if it would still make sense.

The multi-value dropdown component seems slightly similar to what we designed in the mockup, but it is not meant to be used in the way we designed the genomics filter component in the mockup (e.g., we do not want the dropdown menu in the filter). I am not sure of how complex it would be to tune it to our purpose.

  1. Add a new filter when necessary with the sign "+"; otherwise do not show that filter at all. And when folding the filter, it shows a list of the filter name for users to choose. Here is an example of the effect.

I haven't added the "+" button since we have only two filters possible at the moment, so I wasn't sure whether we wanted to spend time now on that functionality.

Regarding showing the list of the filter name for users to choose, I wasn't convinced by its practicality. What happens if the same name is selected in multiple filters? The ideal would be to show names depending on the ones which have been already chosen, but this is a bit more complex implementation.

In general, we can implement all of the above, depending on what we wish to prioritize at the moment. I don't have a very strong opinion about that, what do you think?

@justinjjvanderhooft
Copy link

Thanks for bringing this up! I can think of other filter types (i.e., min / max number of BGCs in the GCF, etc.), so having the option to extent this would be good, but the main goal is to facilitate the integrative analysis of the NPLinker results, so let's focus on that part.
When you say "The ideal would be to show names depending on the ones which have been already chosen, but this is a bit more complex implementation." --> this is generally what we aim for, right?. when doing the integrative analysis (i.e., live subsetting of the data). When you say "a bit more complex implementation", will it take much more time? If it doesn't take too much time, I would definitively consider it, as it would give the web app more practical use.

@CunliangGeng
Copy link
Member

CunliangGeng commented Jul 31, 2024

@gcroci2

  1. Show a folding sign on the "Genomics filter"

The dbc.Accordion looks promising to meet our requirements. Have a look please.

I haven't added the "+" button since we have only two filters possible at the moment, so I wasn't sure whether we wanted to spend time now on that functionality.

Considering Justin's comment, it would be necessary to extend the types of filters later. So I would prefer adding the "+" at the beginning. And this functionality is also needed in the scoring part.

Regarding showing the list of the filter name for users to choose, I wasn't convinced by its practicality. What happens if the same name is selected in multiple filters? The ideal would be to show names depending on the ones which have been already chosen, but this is a bit more complex implementation.

It is not a problem even if users choose the same filters. No matter what filters users choose and how many they choose, the chosen filters could only do "AND" logical operations (we define it), which I think is enough for subsetting data. Otherwise, we should add logical operation (AND, OR, NOT) for each filter. What do you think @justinjjvanderhooft ?

Copy link
Member

@CunliangGeng CunliangGeng left a comment

Choose a reason for hiding this comment

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

Great changes! I like the new effect.

There are 3 points to improve:

  1. The value field of the default filter is empty. We need to set some default value for it.
    image

  2. The hints do not really help. Better to change them to e.g. for filter GCF ID , the hint could be 1, 2, 3.
    image

  3. The dark mode needs different CSS for better display. You could work on it later in another PR only for dark mode.
    image

@CunliangGeng
Copy link
Member

You may want to try Claude Artifacts to assist you, check the video first.

@CunliangGeng
Copy link
Member

Regarding the bigscape class, let's change it to BGC class, which has 8 values
NRP, Polyketide, RiPP, Terpene, Saccharide, Alkaloid, Other, Unknown.

To get the actual value, you need to use bgc.mibig_bgc_class (https://nplinker.github.io/nplinker/latest/api/genomics/#nplinker.genomics.BGC.mibig_bgc_class). The default value is None, i.e. Unknown.

@gcroci2
Copy link
Contributor Author

gcroci2 commented Aug 6, 2024

You may receive notifications about some new commits, but I will explicitly ask for a review when I am finished. @CunliangGeng

@gcroci2
Copy link
Contributor Author

gcroci2 commented Aug 6, 2024

  1. The value field of the default filter is empty. We need to set some default value for it.

Thanks for noticing, now it's not empty anymore.

  1. The hints do not really help. Better to change them to e.g. for filter GCF ID , the hint could be 1, 2, 3.

Done.

  1. The dark mode needs different CSS for better display. You could work on it later in another PR only for dark mode.

I opened a PR (#25)

@CunliangGeng
Copy link
Member

Regarding the bigscape class, let's change it to BGC class, which has 8 values NRP, Polyketide, RiPP, Terpene, Saccharide, Alkaloid, Other, Unknown.

To get the actual value, you need to use bgc.mibig_bgc_class (https://nplinker.github.io/nplinker/latest/api/genomics/#nplinker.genomics.BGC.mibig_bgc_class). The default value is None, i.e. Unknown.

This is still missing. Only hint for GCF ID was changed.

@gcroci2
Copy link
Contributor Author

gcroci2 commented Aug 7, 2024

This is still missing. Only hint for GCF ID was changed.

I know, it's in the TODOs indeed (see my comment at the PR's beginning). The PR is not ready yet ;)

@gcroci2 gcroci2 marked this pull request as draft August 7, 2024 08:02
@CunliangGeng
Copy link
Member

Nice, changing it to draft is clear to me you are still working on it; otherwise it's hard to figure out if it's done or not.

@gcroci2 gcroci2 marked this pull request as ready for review August 7, 2024 14:52
Copy link
Member

@CunliangGeng CunliangGeng left a comment

Choose a reason for hiding this comment

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

The layout looks great now! There are a few more comments about the code.

app/callbacks.py Outdated Show resolved Hide resolved
app/layouts.py Outdated Show resolved Hide resolved
app/layouts.py Show resolved Hide resolved
app/callbacks.py Outdated Show resolved Hide resolved
app/callbacks.py Show resolved Hide resolved
Copy link
Member

@CunliangGeng CunliangGeng left a comment

Choose a reason for hiding this comment

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

Ready to merge after solving the last minor comments 🎉

app/config.py Show resolved Hide resolved
app/config.py Outdated Show resolved Hide resolved
app/callbacks.py Outdated Show resolved Hide resolved
app/callbacks.py Outdated Show resolved Hide resolved
app/callbacks.py Outdated Show resolved Hide resolved
app/callbacks.py Outdated Show resolved Hide resolved
Copy link
Member

@CunliangGeng CunliangGeng left a comment

Choose a reason for hiding this comment

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

Oh yeah! the final of the final review :-)

app/app.py Outdated Show resolved Hide resolved
app/callbacks.py Outdated Show resolved Hide resolved
app/callbacks.py Show resolved Hide resolved
@CunliangGeng
Copy link
Member

🟢 Now you can merge it!

Copy link

sonarcloud bot commented Aug 13, 2024

@gcroci2
Copy link
Contributor Author

gcroci2 commented Aug 13, 2024

🟢 Now you can merge it!

I had to fix the tests first, now I'll merge :)

@gcroci2 gcroci2 merged commit 782c16e into main Aug 13, 2024
3 checks passed
@gcroci2 gcroci2 deleted the 18_genomics_filter_gcroci2 branch August 13, 2024 08:48
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.

Create genomics filter
3 participants