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

feat: fix ontology tree ui performance and enabled large disease ontologies filter on catalogue #4539

Merged
merged 20 commits into from
Jan 12, 2025

Conversation

mswertz
Copy link
Member

@mswertz mswertz commented Dec 3, 2024

closes https://github.com/molgenis/GCC/issues/818
also in parts addresses https://github.com/molgenis/GCC/issues/819 (duplicate PR)

how to test:

N.B. with #4584 we should be able to use the emitSelectedChildren=false. Should probably enable it still in this PR, but added the feature to make it possible so I can continue in the other PR

todo:

  • added population.mainMedicalCondition filter on resources list view
  • addressed performance issue with showing large ontologies by changing Tree component
  • moved search from Ontology to Tree to also address performance issue (and drop the modal, I don't like it)
  • changed ontology options search from modal to inline

N.B. this is confusing, there can also be populationDisease. We would need to produce filter on both with an 'or'? Or shall we reduce the data model so there is only one place where this is defined? I propose on the subpopulations.

@mswertz
Copy link
Member Author

mswertz commented Dec 7, 2024

okay, this takes too long, right?

@mswertz
Copy link
Member Author

mswertz commented Dec 16, 2024

This one blocks on reducing diseases list? Otherwise I propose we don't do this now and first create ability to deal with very large ontologies

@hslh
Copy link
Contributor

hslh commented Dec 16, 2024

If the only way to get the behaviour requested by Kim is to remove items from the ontology, I don't think we should do it. Then it's indeed better to figure out a more general way to deal with large ontologies.

@EleanorHyde-UMCG
Copy link
Contributor

EleanorHyde-UMCG commented Dec 18, 2024

In de directory onder ‘Diagnosis available’ wordt ICD-10 in alle lagen gelijk getoond, ook nog ORPHA gelijk (aparte tabblad maar alles wordt tegelijkertijd geladen) --> waarom is het sneller bij de directory? @mswertz

@connoratrug
Copy link
Contributor

this takes long due to the large tree size, this can be divided into 3 separate issues:

  1. the whole tree added to the dom ( can change this to lazy)
  2. the tree builder uses full scan to find tree elements ( can build map once and use it as loopup list)
  3. the tree is reactive ( primarily for search, but also because the whole app is reactive
  4. heavy data fetch ( fetch all tree nodes)

(5. unbalanced tree ( i a single child list contains a large percentage of the tree items, lazy does not help once this list is visible ) )

1 and 2 can be 'fixed' pretty easy ,
3. means breaking out the vue reactive system, doing some work , and then re-attach then is more work and breaks with the whole reactive app architecture.
4. requires changes to the filter api to fetch root nodes
6. requires a 'paged'/'load more ' lazy loading of a single child list ( this might impact ui/ux ( controles, in controles, in ...), and and quickly get cultured .

the best way imo is to avoid having the vue/js code doing the heavy stuff ( this means lazy tree , only add visible nodes to the dom , filter in backend )

@mswertz
Copy link
Member Author

mswertz commented Dec 28, 2024

this takes long due to the large tree size, this can be divided into 3 separate issues:

  1. the whole tree added to the dom ( can change this to lazy)
  2. the tree builder uses full scan to find tree elements ( can build map once and use it as loopup list)
  3. the tree is reactive ( primarily for search, but also because the whole app is reactive
  4. heavy data fetch ( fetch all tree nodes)

(5. unbalanced tree ( i a single child list contains a large percentage of the tree items, lazy does not help once this list is visible ) )

1 and 2 can be 'fixed' pretty easy , 3. means breaking out the vue reactive system, doing some work , and then re-attach then is more work and breaks with the whole reactive app architecture. 4. requires changes to the filter api to fetch root nodes 6. requires a 'paged'/'load more ' lazy loading of a single child list ( this might impact ui/ux ( controles, in controles, in ...), and and quickly get cultured .

the best way imo is to avoid having the vue/js code doing the heavy stuff ( this means lazy tree , only add visible nodes to the dom , filter in backend )

Indeed, the (1) and (2) were most easy. The 'find' that was what slowed it down most dramatically. In addition using 'show' instead 'if' for the invisible nodes indeed also lost some performance.

Replacing it with implementation that is more like the original molgenis-components fixed that issue and I think makes it acceptable. (I think the molgenis-components implementation of the tree has another benefit which is for showing the 'partial' on the parent of the parent, but that refactoring can be done later).

P.S. If we want to make it feel really snappy we could load also the hidden filter items, then the render is fast (fetch is the remaining time now). Alternatively, indeed do a partial load of only the 'roots' and then load subtrees as suggested in (4). Would require some extra backend filter options (e.g. retrieve where parent==null) and ability to delegate subtree filtering to backend (now we simply dump all subitems into the filter).

@mswertz
Copy link
Member Author

mswertz commented Dec 28, 2024

Search in the ontology is not fast enough still. I don't understand why we copy the whole tree instead of simply annotating it with a 'visible' just as with molgenis-components. That would prevent complete reloading but simply hide elements.

@mswertz
Copy link
Member Author

mswertz commented Dec 29, 2024

Search in the ontology is not fast enough still. I don't understand why we copy the whole tree instead of simply annotating it with a 'visible' just as with molgenis-components. That would prevent complete reloading but simply hide elements.

also fixed now.

Copy link
Contributor

Choose a reason for hiding this comment

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

why an extra file when the tree itself is a node

Copy link
Member Author

Choose a reason for hiding this comment

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

tree state should be managed once for the whole Tree instead of per TreeNode.

Copy link
Contributor

Choose a reason for hiding this comment

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

the state can be managed outside of the tree , the tree can render the state

@connoratrug connoratrug changed the title feat(catalogue): add disease filter to resources list page fix: ontology tree ui performance Dec 30, 2024
Copy link
Contributor

@connoratrug connoratrug left a comment

Choose a reason for hiding this comment

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

  • the tree selection ( deselection ) is incorrect
  • tests are missing

@mswertz mswertz changed the title fix: ontology tree ui performance feat: ontology tree ui performance and enabled large disease ontologies filter on catalogue Dec 30, 2024
@mswertz
Copy link
Member Author

mswertz commented Dec 30, 2024

  • the tree selection ( deselection ) is incorrect
  • tests are missing

Can you be more specific? I believe the selection/deselection is correct. Can you give an example to reproduce?

There were no tests before, so I cannot be bothered to add them now on this change.

@mswertz
Copy link
Member Author

mswertz commented Dec 30, 2024

Can you be more specific? I believe the selection/deselection is correct. Can you give an example to reproduce?

I think I found it, updating the Tree.story for easy testing.

@mswertz
Copy link
Member Author

mswertz commented Dec 30, 2024

There were no tests before, so I cannot be bothered to add them now on this change.

I updated the Tree.story

@mswertz mswertz changed the title feat: ontology tree ui performance and enabled large disease ontologies filter on catalogue feat: fix ontology tree ui performance and enabled large disease ontologies filter on catalogue Dec 30, 2024
@mswertz mswertz requested a review from connoratrug December 30, 2024 11:09
@mswertz mswertz dismissed connoratrug’s stale review January 7, 2025 11:12

I addressed most of the comments, just the test: would an e2e test be suitable place?

@mswertz mswertz merged commit 4df7d87 into master Jan 12, 2025
6 of 7 checks passed
@mswertz mswertz deleted the feat/icd10filter_resources branch January 12, 2025 19:25
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.

4 participants