-
Notifications
You must be signed in to change notification settings - Fork 39
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
Daskify NIR reflectance calculations #59
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
d94cb88
Use dask instead of Numpy where suitable
pnuu 4b88bdb
Use np.testing.assert_allclose instead of assertAlmostEqual
pnuu a695588
Revert back to use numpy operations
pnuu 71ff135
Use np.asanyarray instead of np.array
pnuu 45e38b1
Remove references to masked array attributes
pnuu 654e3e7
Change documentation to use plain Numpy arrays instead of masked arrays
pnuu 01933a7
Remove obsolete commented-out lines
pnuu b977c21
Don't rely on dask to be available
pnuu 92b743c
Compute the results if the input is not dask array
pnuu b22d176
Fix da.log and da.exp to np.log and da.log
pnuu 500dc7f
Convert the result to masked array if also the input is masked
pnuu 781e477
Add tests for checking output for masked and dask arrays stay the same
pnuu 86086df
Ensure that radiance from planck() isn't masked
pnuu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
np.asanyarray
convert dask arrays to numpy arrays?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djhoese what do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned on slack, I'd prefer if everything could be kept as dask arrays for as long as possible and if things have to be computed they should be done in a
map_blocks
ordelayed
function. As @pnuu mentioned on slack converting to numpy here made the code faster and use less memory in his test cases. I'm fairly certain this is because the array is used later on in a non-dask-friendly way and the dask array is actually being computed multiple times. I think I pointed them out on slack during the PCW.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this part of pyspectral wasn't dask friendly before, could we perhaps merge this PR now and make sure things works and then work further to improve the daskfriendliness?
@mraspaud @djhoese @pnuu ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with merging. Then we could also merge pytroll/satpy#529 so this PR would be usable via SatpY.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is working now ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there hasn't been anything that didn't work since I last touched this, just that it might not be quite optimal in performance and how things should be done with dask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine. +0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine, I will just try do some system testing off line first to be sure. Any suggested recipes/tests @pnuu ?