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

Move calculation of metrics to dm_pix #58

Merged
merged 17 commits into from
Sep 20, 2024
Merged

Move calculation of metrics to dm_pix #58

merged 17 commits into from
Sep 20, 2024

Conversation

phinate
Copy link
Collaborator

@phinate phinate commented Sep 13, 2024

Overhauls our metric calculations to use JAX and DeepMind's pix library. Working from a pipeline perspective, which is great!

Issues right now:

  • SSIM is producing NaNs (I've added a test to verify this)
  • I've forked the repo to replace mean -> nanmean, but I haven't worked out how to fix this for SSIM. Will require an explicit install:
pip install git+https://github.com/phinate/dm_pix@ignore-nans

or ideally, just clone my fork and make some edits if you want to help debug this. I've added @dfulu as a collaborator since he's been working on metrics the most with me, but any help is appreciated!

@phinate phinate marked this pull request as ready for review September 16, 2024 16:31
@phinate
Copy link
Collaborator Author

phinate commented Sep 16, 2024

Problems are solved, so have marked this ready for review.

Still need to add install instructions for the fork of dm_pix. We could possibly add it as a submodule. Open to thoughts?

@dfulu
Copy link
Collaborator

dfulu commented Sep 17, 2024

Problems are solved, so have marked this ready for review.

Still need to add install instructions for the fork of dm_pix. We could possibly add it as a submodule. Open to thoughts?

From what I can tell, it seems like we are only dependent on this one module of dm_pix and its only around 250 lines of code. Should we just copy that into this repo rather than remaining dependent on that external fork?

@phinate
Copy link
Collaborator Author

phinate commented Sep 17, 2024

Should we just copy that into this repo rather than remaining dependent on that external fork?

I also thought this. I guess I didn't know if we'd want to use more functionality from the lib. Happy to take that approach for now, since it works nicely for the medium-term.

@phinate
Copy link
Collaborator Author

phinate commented Sep 17, 2024

Have added the relevant code to our repo, keeping the name metrics.py, but importing as dm_pix where used for forward compatibility.

TL;DR on what the problems were: our tests failed because the filter size for the convolution in metrics.ssim was bigger than the image; the nan in the top-left corner made the whole image be replaced with nans. This is solved by just reducing the filter size during tests (our images there are only 8x9 to speed up computations, so our filter size needs to be around 9 or less, instead of the default of 11).

@phinate
Copy link
Collaborator Author

phinate commented Sep 17, 2024

Note the extra install step for running things on gpu (now in README):

python -m pip install --upgrade "jax[cuda12]"

@phinate phinate requested a review from dfulu September 17, 2024 12:32
README.md Show resolved Hide resolved
tests/test_metrics.py Show resolved Hide resolved
src/cloudcasting/validation.py Show resolved Hide resolved
README.md Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 82.60870% with 20 lines in your changes missing coverage. Please review.

Project coverage is 91.71%. Comparing base (cd29ad7) to head (c32d074).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
src/cloudcasting/metrics.py 77.50% 18 Missing ⚠️
src/cloudcasting/validation.py 93.54% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
- Coverage   94.88%   91.71%   -3.17%     
==========================================
  Files          11       11              
  Lines         469      531      +62     
==========================================
+ Hits          445      487      +42     
- Misses         24       44      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phinate phinate merged commit 6539034 into main Sep 20, 2024
7 checks passed
@phinate phinate deleted the gpu-metrics branch September 20, 2024 10:17
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.

3 participants