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

Adding high level plotting API #128

Merged
merged 41 commits into from
Dec 9, 2022
Merged

Adding high level plotting API #128

merged 41 commits into from
Dec 9, 2022

Conversation

manuguth
Copy link
Contributor

@manuguth manuguth commented Sep 19, 2022

Summary

This pull request introduces the following changes

  • Adds a new high level plotting API (hlplots) to be able to plot ROC curves etc. with less lines of code

Relates to the following issues

TODOs

  • Add full integration for c-tagging
  • add all the tests for the results module
  • add documentation

Conformity

@manuguth manuguth changed the title Manuguth hlapi Adding high level plotting API Sep 19, 2022
@samvanstroud
Copy link
Contributor

Hi @manuguth, any update on this?

@manuguth
Copy link
Contributor Author

manuguth commented Oct 20, 2022

Hi @manuguth, any update on this?

It's on my ToDo, hope to get to it soon
the skeleton is there, it needs a bit of tuning

Copy link
Contributor

@samvanstroud samvanstroud left a comment

Choose a reason for hiding this comment

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

keen to see this merged! I had a couple of discussion points.

examples/high_level_plots.py Outdated Show resolved Hide resolved
puma/hlplots/tagger.py Outdated Show resolved Hide resolved
puma/hlplots/results.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

Merging #128 (e2d516e) into main (c4b4cac) will increase coverage by 0.72%.
The diff coverage is 99.13%.

@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
+ Coverage   95.78%   96.50%   +0.72%     
==========================================
  Files          23       30       +7     
  Lines        2206     2663     +457     
==========================================
+ Hits         2113     2570     +457     
  Misses         93       93              
Impacted Files Coverage Δ
puma/tests/utils/test_generate.py 100.00% <ø> (ø)
puma/utils/generate.py 100.00% <ø> (ø)
puma/hlplots/results.py 97.82% <97.82%> (ø)
puma/hlplots/tagger.py 98.46% <98.46%> (ø)
puma/tests/hlplots/test_results.py 99.25% <99.25%> (ø)
puma/hlplots/__init__.py 100.00% <100.00%> (ø)
puma/tests/hlplots/test_tagger.py 100.00% <100.00%> (ø)
puma/tests/utils/test_discriminant.py 100.00% <100.00%> (ø)
puma/utils/__init__.py 71.05% <100.00%> (+0.78%) ⬆️
puma/utils/discriminant.py 100.00% <100.00%> (ø)
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@manuguth
Copy link
Contributor Author

@samvanstroud @jobirk @afroch @philippgadow I now have a first fully implemented suggestion for the high level API (some small things are still missing though most of it is there).
Could you give me feedback about the current implementation? Afterwards I would finalise the tests, docs etc

examples/high_level_plots.py Show resolved Hide resolved
examples/high_level_plots.py Show resolved Hide resolved
puma/hlplots/results.py Outdated Show resolved Hide resolved
puma/hlplots/results.py Outdated Show resolved Hide resolved
puma/hlplots/results.py Outdated Show resolved Hide resolved
puma/hlplots/tagger.py Show resolved Hide resolved
puma/tests/hlplots/test_results.py Outdated Show resolved Hide resolved
puma/utils/discriminant.py Outdated Show resolved Hide resolved
puma/utils/discriminant.py Outdated Show resolved Hide resolved
puma/utils/discriminant.py Outdated Show resolved Hide resolved
@afroch
Copy link
Contributor

afroch commented Nov 22, 2022

Ok so overall I really like the structure of the new API. I think it's really nice to have it that way

puma/utils/discriminant.py Outdated Show resolved Hide resolved
puma/hlplots/tagger.py Outdated Show resolved Hide resolved
examples/high_level_plots.py Show resolved Hide resolved
puma/hlplots/results.py Outdated Show resolved Hide resolved
puma/hlplots/tagger.py Outdated Show resolved Hide resolved
puma/tests/utils/test_generate.py Outdated Show resolved Hide resolved
puma/tests/utils/test_generate.py Outdated Show resolved Hide resolved
puma/utils/discriminant.py Outdated Show resolved Hide resolved
puma/utils/discriminant.py Show resolved Hide resolved
@joschkabirk
Copy link
Contributor

Really like the implementation 🎉

puma/hlplots/tagger.py Outdated Show resolved Hide resolved
puma/hlplots/tagger.py Show resolved Hide resolved
puma/hlplots/tagger.py Show resolved Hide resolved
puma/hlplots/tagger.py Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
puma/hlplots/tagger.py Outdated Show resolved Hide resolved
@manuguth manuguth mentioned this pull request Dec 9, 2022
5 tasks
@manuguth manuguth marked this pull request as ready for review December 9, 2022 14:36
examples/high_level_plots.py Outdated Show resolved Hide resolved
puma/hlplots/results.py Outdated Show resolved Hide resolved
puma/hlplots/results.py Outdated Show resolved Hide resolved
@manuguth manuguth merged commit 4a6613f into main Dec 9, 2022
@manuguth manuguth deleted the manuguth-hlapi branch December 9, 2022 15:39
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.

5 participants