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

[Future Spherical Class] Add SPoint and SMultiPoint #465

Merged
merged 20 commits into from
Nov 17, 2022

Conversation

ghiggi
Copy link
Contributor

@ghiggi ghiggi commented Nov 8, 2022

This PR add the SPoint and SMultiPoint class of the future spherical geometry interface.

  • Tests added
  • Tests passed
  • Passes git diff origin/main **/*py | flake8 --diff

@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #465 (a48ac65) into main (5f53ecd) will decrease coverage by 0.03%.
The diff coverage is 92.96%.

@@            Coverage Diff             @@
##             main     #465      +/-   ##
==========================================
- Coverage   94.27%   94.24%   -0.04%     
==========================================
  Files          69       73       +4     
  Lines       12394    12703     +309     
==========================================
+ Hits        11685    11972     +287     
- Misses        709      731      +22     
Flag Coverage Δ
unittests 94.24% <92.96%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyresample/spherical.py 92.67% <80.17%> (-5.55%) ⬇️
pyresample/future/spherical/__init__.py 100.00% <100.00%> (ø)
pyresample/future/spherical/point.py 100.00% <100.00%> (ø)
pyresample/test/test_sgeom/__init__.py 100.00% <100.00%> (ø)
pyresample/test/test_sgeom/test_point.py 100.00% <100.00%> (ø)
pyresample/test/test_spherical.py 100.00% <100.00%> (ø)
pyresample/_multi_proc.py 83.01% <0.00%> (+1.88%) ⬆️

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

@coveralls
Copy link

coveralls commented Nov 8, 2022

Coverage Status

Coverage remained the same at 93.761% when pulling a48ac65 on ghiggi:feature-spoint into 5f53ecd on pytroll:main.

@ghiggi
Copy link
Contributor Author

ghiggi commented Nov 9, 2022

@djhoese @mraspaud this is ready for review.
Tomorrow I have faculty meetings, but I can finalize the last details on Friday if you manage to review it tomorrow.
This PR is required for the next SArc class PR

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Thanks for taking on the job of renovating this part of Pyresample, it is highly needed! Some suggestions, questions and comments inline.

pyresample/future/spherical/point.py Outdated Show resolved Hide resolved
pyresample/future/spherical/point.py Outdated Show resolved Hide resolved
pyresample/future/spherical/point.py Outdated Show resolved Hide resolved
pyresample/future/spherical/point.py Outdated Show resolved Hide resolved
pyresample/future/spherical/point.py Outdated Show resolved Hide resolved
pyresample/spherical.py Outdated Show resolved Hide resolved
pyresample/spherical.py Show resolved Hide resolved
pyresample/spherical.py Show resolved Hide resolved
pyresample/spherical.py Outdated Show resolved Hide resolved
pyresample/test/test_spherical.py Show resolved Hide resolved
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Thanks, starts to look good.

pyresample/spherical.py Outdated Show resolved Hide resolved
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

This is looking really good. I haven't kept up on this development so I had a lot of questions and a couple requested changes, but otherwise I like how you cleaned up the code.

One other suggestion that may help tests in the future, should some of these SCoordinate objects you use in the tests be pytest fixtures or other helper methods so the code to generate them doesn't get repeated all over the tests? I see some of them are unique, but also some use the same angles as inputs.

Again, nice job, just a few more things.

pyresample/future/spherical/point.py Outdated Show resolved Hide resolved
pyresample/future/spherical/point.py Outdated Show resolved Hide resolved
pyresample/future/spherical/point.py Outdated Show resolved Hide resolved
pyresample/future/spherical/point.py Outdated Show resolved Hide resolved
pyresample/future/spherical/point.py Outdated Show resolved Hide resolved
pyresample/spherical.py Outdated Show resolved Hide resolved
pyresample/spherical.py Show resolved Hide resolved
pyresample/spherical.py Outdated Show resolved Hide resolved
pyresample/spherical.py Outdated Show resolved Hide resolved
pyresample/test/test_sgeom/__init__.py Show resolved Hide resolved
@ghiggi
Copy link
Contributor Author

ghiggi commented Nov 16, 2022

@mraspaud @djhoese ready for merging I guess ;)

@ghiggi
Copy link
Contributor Author

ghiggi commented Nov 16, 2022

I just added a small class method from_degrees to avoid calling np.deg2rad in many occasions when creating the object

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

This looks good enough for me that it shouldn't be holding up any other PRs. I had one comment about something I wouldn't mind changed, but it isn't as big of a deal as the other stuff you've already fixed.

pyresample/future/spherical/point.py Show resolved Hide resolved
@mraspaud mraspaud closed this Nov 17, 2022
@mraspaud mraspaud reopened this Nov 17, 2022
@mraspaud mraspaud merged commit 952cec6 into pytroll:main Nov 17, 2022
@ghiggi ghiggi deleted the feature-spoint branch November 17, 2022 09:32
@djhoese djhoese added this to the v2.0 milestone Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants