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

improve: update InfoLM class to dynamically set higher_is_better #2674

Merged
merged 15 commits into from
Aug 30, 2024

Conversation

grahamannett
Copy link
Contributor

@grahamannett grahamannett commented Aug 3, 2024

…ed on information_measure

What does this PR do?

Fixes #2659

Before submitting
  • Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

I had problems with the devcontainer and the environment.

Might try and add tests for this but it seems the devcontainer uses the torchmetrics in /usr/local/lib/python3.9/site-packages (or similar depending on python) and its not obvious to me how you are supposed to setup things to easily/quickly add tests? I ended up just doing editable install.

I manually checked all the values but if I add a test ill try and include some info/example for each of these that shows the metric output on strs.


📚 Documentation preview 📚: https://torchmetrics--2674.org.readthedocs.build/en/2674/

@Borda Borda changed the title refactor: update InfoLM class to dynamically set higher_is_better bas… improve: update InfoLM class to dynamically set higher_is_better Aug 5, 2024
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 39%. Comparing base (79b33ce) to head (af2b87e).
Report is 105 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (79b33ce) and HEAD (af2b87e). Click for more details.

HEAD has 80 uploads less than BASE
Flag BASE (79b33ce) HEAD (af2b87e)
Windows 6 3
python3.8 6 3
cpu 40 20
torch1.13.1+cpu 6 3
torch1.12.1+cpu 2 1
Linux 26 13
python3.9 18 9
torch1.11.0+cpu 2 1
torch1.10.2+cpu 2 1
macOS 8 4
python3.11 6 3
torch2.4.0 2 1
torch2.4.0+cpu 2 1
python3.10 10 5
torch2.0.1 4 2
torch2.0.1+cpu 6 3
torch2.3.1+cpu 4 2
torch2.2.2+cpu 4 2
torch2.1.2+cpu 2 1
torch1.13.1 2 1
torch2.4.0+cu121 2 1
Additional details and impacted files
@@           Coverage Diff            @@
##           master   #2674     +/-   ##
========================================
- Coverage      69%     39%    -30%     
========================================
  Files         316     316             
  Lines       17897   17900      +3     
========================================
- Hits        12325    7027   -5298     
- Misses       5572   10873   +5301     

@SkafteNicki SkafteNicki added this to the v1.5.0 milestone Aug 27, 2024
@SkafteNicki SkafteNicki added enhancement New feature or request bug / fix Something isn't working labels Aug 27, 2024
@grahamannett
Copy link
Contributor Author

Im still not sure where to put the info that was requested for the higher is better but incorporated the changes asked for the @propertyrelated bit. if you can outline/direct what you would like I will incorporate the changes but seems likely the ways I would decide to implement it/where to store the info isnt what @Borda wants

@SkafteNicki
Copy link
Member

@Borda can you give more specific instructions on what you want changed here?

@SkafteNicki
Copy link
Member

@Borda should be good to go now

@mergify mergify bot added the ready label Aug 29, 2024
@Borda Borda merged commit c233f36 into Lightning-AI:master Aug 30, 2024
47 of 55 checks passed
@grahamannett grahamannett deleted the fix/infolm-higher-is-better branch August 30, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / fix Something isn't working enhancement New feature or request ready topic: Text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

higher_is_better for InfoLM should change depending on information_measure
4 participants