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

Multispectral tools: convert data to float32 dtype before doing calculations #755

Merged
merged 2 commits into from
Feb 16, 2023

Conversation

thuydotm
Copy link
Contributor

@thuydotm thuydotm commented Jan 13, 2023

Fixes #726.

TODO:

  • add tests

@thuydotm thuydotm added the ready to merge PR is ready to merge label Jan 13, 2023
@thuydotm thuydotm requested a review from brendancol January 13, 2023 15:21
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2023

Codecov Report

Base: 79.88% // Head: 79.88% // No change to project coverage 👍

Coverage data is based on head (bb04f35) compared to base (af42544).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #755   +/-   ##
=======================================
  Coverage   79.88%   79.88%           
=======================================
  Files          19       19           
  Lines        4170     4170           
=======================================
  Hits         3331     3331           
  Misses        839      839           
Impacted Files Coverage Δ
xrspatial/multispectral.py 69.76% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@thuydotm thuydotm added WIP Work in progress and removed ready to merge PR is ready to merge labels Jan 13, 2023
Copy link
Contributor

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

I know f32 is ideal for performance. But this sounds a little dangerous if the user is assuming there data stays as the same type during calculation. Could add a little more detail on why this is important to cast?

@thuydotm
Copy link
Contributor Author

Almost our tools return data of dtype f32. Although the calculations on f32 can cause some minor numerical error if the original data is of f64, we save the result in f32 I think those errors are acceptable. In case of issue #726, converting from uint16 to floating point dtype (f32 specifically) is necessary to make sure result of a calculation does not exceed the bounds of unsigned int data type.

@thuydotm thuydotm added ready to merge PR is ready to merge and removed WIP Work in progress labels Jan 18, 2023
@brendancol
Copy link
Contributor

@thuydotm Can you follow up with a PR with documentation of how these types are handled?

#766

@brendancol brendancol merged commit 68aa763 into master Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge PR is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overflow error with NDVI calculation
3 participants