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

create selected data table #30

Merged
merged 26 commits into from
Oct 1, 2024
Merged

Conversation

gcroci2
Copy link
Contributor

@gcroci2 gcroci2 commented Aug 14, 2024

With this PR, an interactive DataTable displaying the data filtered via the genomics filter is added.

  • The filters are applied live.
  • The filters are put in a OR operation.
  • Multiple rows can be selected.
  • A practical button for selecting/deselecting all rows has been added.
  • The pages' numbering is automatically handled for bigger datasets.
  • Two output placeholders have been placed after the DataTable but they will be removed in the next step, when the scoring part will be inserted. Now they are useful for dev purposes.
  • Now instead of storing the uploaded file path, we store a json file which is lighter and contain only the information we need so far. Otherwise, in each callback that uses the data we would have to upload the .pkl file again.
  • Tests have been added.

The hyperlinks we designed (see #21) are still missing, but I think that this PR can already be reviewed before going on and finalizing it.

@gcroci2 gcroci2 linked an issue Aug 14, 2024 that may be closed by this pull request
@gcroci2 gcroci2 marked this pull request as draft August 14, 2024 14:58
@gcroci2 gcroci2 marked this pull request as ready for review August 15, 2024 14:00
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.

Nice changes! Before reviewing the details of the code, I have some suggestions (see figure below):

  1. Add OR label to each filter?
  2. Change the button Select/Deselect all to a selection box?

image

@gcroci2
Copy link
Contributor Author

gcroci2 commented Aug 28, 2024

  1. Add OR label to each filter?

Added :)

  1. Change the button Select/Deselect all to a selection box?

Done. I first tried to directly edit the first column name of the DataTable, but that was not doable since the "selection" column is not an easily accessible one (it's created by setting row_selectable="multi" in the DataTable component, so we don't have direct access to it for editing), and on top of this columns headers seem to require strings, and do not accept other Dash components. So I ended up tweaking the dcc.Checklist component style to move it in the place you suggested.

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.

Nice changes, I like it.

It looks the filter of BGC class does not work properly. For GCFs that do not have a BGC class should have a label Unknown, but if you check your test data mock_obj_data.pkl, Unknown gives nothing. Or you can try selecting all labels in BGC class, which does not give all data available.

requirements.txt Outdated Show resolved Hide resolved
@gcroci2
Copy link
Contributor Author

gcroci2 commented Sep 9, 2024

It looks the filter of BGC class does not work properly. For GCFs that do not have a BGC class should have a label Unknown, but if you check your test data mock_obj_data.pkl, Unknown gives nothing. Or you can try selecting all labels in BGC class, which does not give all data available.

The issue is fixed now—thanks for pointing it out, @CunliangGeng! It was caused by a mix-up between capitalized and non-capitalized class names.

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.

It works! :shipit:

One thought about plotting: currently we provide one plot for #BGC -> #GCF, users may also want other plots such as BGC class -> #GCF.
I'm thinking, instead of providing one or two plots beforehand, we leave it for users to do it:

  • we provide the options for x and y, e.g. #GCF, #BGC, BGC class, for users to choose
  • we provide different plot styles for users to choose, e.g. histogram, box plot
  • users choose those and click plot, then get whatever plots they want
    We don't have to implement it now, and could ask for feedbacks on this idea on the community meeting.

@gcroci2
Copy link
Contributor Author

gcroci2 commented Sep 10, 2024

I found a bug: if we filter a GCF ID, then we select it from the table, and then we cancel it from the filter to insert other ids, and we place another id/ids, then the first one will result selected in the table, even though we didn't select it.

Other useful genomics filters to add: BGC ID, strain

@gcroci2
Copy link
Contributor Author

gcroci2 commented Sep 11, 2024

We have now some nice ideas to further improve the plot functionality. Which ones to we want to pick up in the current PR? @CunliangGeng

To recap, the following are the "mandatory" todos:

And the following are nice additions to decide priority on:

  • Add options for x and y axis, e.g. #GCF, #BGC, BGC class, for users to choose
  • Provide different plot styles for users to choose, e.g. histogram, box plot
  • Add BGC ID and strain filters

I will then open new issues, if needed, accordingly.

@CunliangGeng
Copy link
Member

Thanks for the recap @gcroci2. It's good to merge the current PR and open new ones for the rest issues.

As for the priority, I think it's better to finish the whole page before adding new features like custom plot:

  • Add BGC ID and strain filters

  • add scoring part

  • custom plot

    • Add options for x and y axis, e.g. #GCF, #BGC, BGC class, for users to choose
    • Provide different plot styles for users to choose, e.g. histogram, box plot

What do you think?

@gcroci2
Copy link
Contributor Author

gcroci2 commented Sep 13, 2024

What do you think?

I created a new view for the webapp, see here. I labeled there as priorities the issues for finishing the genomics -> metabolomics tab. Then the other issues in "Todo" can be picked up, and the ones with "ehnancement" label (such as the plotting customization) will be the latter to be picked up. Everything is in order of priority, from top to bottom.

So next week I will wrap up this PR, in particular by adding the hyperlinks to the table's elements (with at least some basic info displayed), and by fixing the filtering/selection bug I mentioned above.

@gcroci2
Copy link
Contributor Author

gcroci2 commented Sep 19, 2024

We decided to add a filter button to display the filtered data on the DataTable instead of allowing the user to go back and forth between the filter and the DataTable. Every time that the button is clicked, the selection gets cleaned.

@gcroci2 gcroci2 closed this Sep 19, 2024
@gcroci2 gcroci2 reopened this Sep 19, 2024
@gcroci2
Copy link
Contributor Author

gcroci2 commented Sep 23, 2024

Almost done! @CunliangGeng

  • I resolved the issue with filtering and row selection by adding a button to apply the filters. Each time the button is clicked, the DataTable row selection is reset. The select-all checkbox functionality is also improved for better consistency.
  • I implemented tooltips using the DataTable’s tooltip_data feature. For now, I’ve arbitrarily set the GCF's strains attribute to display in the GCF IDs column and the BGCs' id attribute in the "# BGC" column. Let me know if we want to display additional information or if I should open an issue for future enhancements.
    • Initially, I explored using dbc.Tooltip component for greater flexibility. However, dynamically adding a tooltip to each row/column element would require individual element IDs to be passed to the "target" parameter of dbc.Tooltip. After some investigation, I couldn’t find a straightforward way to achieve this, and it seems that DataTable doesn't support such dynamic elements (without embedding some Javascript on the client side).
    • Should I invest more time exploring the dbc.Tooltip option, or should I open a future issue for this enhancement? The key advantage of dbc.Tooltip is its flexibility, allowing us to embed complex content and enable user interaction, features not supported by the built-in DataTable tooltips.

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.

Looks good to me!

We could keep the current tooltips, but it's better to

  1. display the text in a table format for strain IDs and BGC IDs (a one column table, later we can add more columns)?
  2. the tooltips can be displayed but will disappear when I try to select and copy text from it. Is there a way to do that?

@gcroci2
Copy link
Contributor Author

gcroci2 commented Sep 24, 2024

  1. display the text in a table format for strain IDs and BGC IDs (a one column table, later we can add more columns)?

I did it using a markdown type of text, since the tooltip_data parameter supports only plain text and markdown-formatted text.

  1. the tooltips can be displayed but will disappear when I try to select and copy text from it. Is there a way to do that?

I am aware of this issue, but I did't find anything to handle it using such tooltips.

Both 1. and 2. kind of enhancements were among the reasons why I wanted to use a dbc.Tooltip rather than the ones built-in DataTables, which have much more simpler functionalities and so are quite limited. But dbc.Tooltip have not been designed to be incorporated in DataTable rows' elements.

@CunliangGeng
Copy link
Member

CunliangGeng commented Sep 25, 2024

I did it using a markdown type of text, since the tooltip_data parameter supports only plain text and markdown-formatted text.

Cool!

Could you change the font or the background to make the tooltip box more visible/highlighted? Maybe make the font smaller or using a colourful background?

For the column Strains, better to directly display the strain names (ordered) instead of a StrainCollection.
For the BCG table, also better to make the column ordered by e.g. alphabet.

I am aware of this issue, but I did't find anything to handle it using such tooltips.

OK, let's record it as an issue and improve it later.

Copy link

sonarcloud bot commented Sep 27, 2024

@gcroci2
Copy link
Contributor Author

gcroci2 commented Sep 27, 2024

Could you change the font or the background to make the tooltip box more visible/highlighted? Maybe make the font smaller or using a colourful background?

For the column Strains, better to directly display the strain names (ordered) instead of a StrainCollection.
For the BCG table, also better to make the column ordered by e.g. alphabet.

Done :)

OK, let's record it as an issue and improve it later.

Opened in #35

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.

I like the new style of tooltip. There is one more issue:

When the data table has multiple pages, the display position of tooltip for the page 2+ will be kind of random...
image

@gcroci2
Copy link
Contributor Author

gcroci2 commented Oct 1, 2024

When the data table has multiple pages, the display position of tooltip for the page 2+ will be kind of random...

I noticed that and I've just tried to play around with the css properties, but with no improvements. I've added a bullet point about it in #35

@gcroci2 gcroci2 merged commit 4ed2571 into main Oct 1, 2024
3 checks passed
@gcroci2 gcroci2 deleted the 21_create_selected_table_gcroci2 branch October 1, 2024 13:21
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 selected data table
2 participants