Refactor MODIS readers to avoid extra dask tasks #3081
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
djhoese
added
component:readers
cleanup
djhoese
force-pushed
the
cleanup-modis-dask-tasks
branch
from
946c377 to
8e5ee94
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3081 +/- ##
=======================================
Coverage 96.14% 96.15%
=======================================
Files 383 383
Lines 55798 55813 +15
=======================================
+ Hits 53649 53666 +17
+ Misses 2149 2147 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mraspaud
reviewed
Mar 18, 2025
| @@ -280,7 +280,8 @@ def calibrate_refl(array, attributes, index): | |||
| offset = np.float32(attributes["reflectance_offsets"][index]) | |||
| scale = np.float32(attributes["reflectance_scales"][index]) | |||
| # convert to reflectance and convert from 1 to % | |||
| array = (array - offset) * scale * 100 | |||
| array = (array - offset) | |||
There was a problem hiding this comment.
Suggested change
| array = (array - offset) | |
| array = array - offset |
There was a problem hiding this comment.
Crap. I just realized I never did this.
Pull Request Test Coverage Report for Build 13972719479Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
3 tasks
mraspaud
reviewed
Mar 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was using the
modis_l1breader for debugging something dask related and got annoyed at how many tasks were in the task graph for simple reading functionality. This PR rewrites some of these things to avoid the repetitive tasks (scaling and fill value checks) and the confusing tasks (angle offsets for lonlat2xyz compatibility). I was tempted to also apply changes like this to all the calibration and uncertainty checks, but overall these changes don't affect performance much so I plan to leave these other changes for a future where I (or someone else) is annoyed by all the tasks being shown by dask.Note: I did these while debugging other things so it is possible the code is not the cleanest and needs more cleanup, but the tests pass so I think that's a good start.
AUTHORS.mdif not there already