linkml / linkml-runtime

Runtime support for linkml generated models
https://linkml.io/linkml/
Creative Commons Zero v1.0 Universal
22 stars 21 forks source link

optimize `get_classes_by_slot()` in `schemaview.py` #300

Closed sujaypatil96 closed 4 months ago

sujaypatil96 commented 4 months ago

The motivation to optimize this PR is that we are having to remove the Applicable classes section in the generated slot markdown/web documentation pages: https://github.com/linkml/linkml/blob/main/linkml/generators/docgen/slot.md.jinja2#L36-L46 for large schemas like mixs.

The optimizations that have been applied here are:

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (51d2fce) 62.80% compared to head (54d9515) 62.90%. Report is 5 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #300 +/- ## ========================================== + Coverage 62.80% 62.90% +0.10% ========================================== Files 61 62 +1 Lines 8506 8530 +24 Branches 2239 2239 ========================================== + Hits 5342 5366 +24 - Misses 2553 2554 +1 + Partials 611 610 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sujaypatil96 commented 4 months ago

After wonderful optimizations by Jonny in https://github.com/linkml/linkml-runtime/pull/296 I thought this would be an appropriate time to take advantage of his modifications and apply it to large schemas (like mixs) that need this performance improvement.

sujaypatil96 commented 4 months ago

@cmungall thanks for approving this PR. I'm still benchmarking if it's easier/faster to return ClassDefinition instead of ClassDefinitionName (corresponding changes will be required in slot.md.jinja2).

I won't merge this in till I have results from the benchmarking. I'm experimenting with these changes on the mixs schema.

sujaypatil96 commented 4 months ago

I think we are good to go with this PR as is @cmungall.