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

Replace and deprecate frequency arg for bbox methods #526

Merged
merged 4 commits into from
Aug 15, 2023

Conversation

mraspaud
Copy link
Member

This PR replaces the frequency argument used in many *Definition method with the more correct bbox_shape.
Historically, the name was correct, but a behaviour-changing commit (19e1ea5) it became inaccurate.

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

@mraspaud mraspaud added the bug label Jun 28, 2023
@mraspaud mraspaud self-assigned this Jun 28, 2023
@mraspaud mraspaud mentioned this pull request Jun 28, 2023
4 tasks
@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #526 (bea2050) into main (2a6d769) will decrease coverage by 0.10%.
Report is 12 commits behind head on main.
The diff coverage is 95.12%.

@@            Coverage Diff             @@
##             main     #526      +/-   ##
==========================================
- Coverage   94.34%   94.25%   -0.10%     
==========================================
  Files          82       82              
  Lines       13026    13045      +19     
==========================================
+ Hits        12290    12295       +5     
- Misses        736      750      +14     
Flag Coverage Δ
unittests 94.25% <95.12%> (-0.10%) ⬇️

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

Files Changed Coverage Δ
pyresample/geometry.py 87.63% <95.12%> (+0.04%) ⬆️

... and 2 files with indirect coverage changes

@mraspaud mraspaud requested a review from djhoese June 28, 2023 12:25
@coveralls
Copy link

coveralls commented Jun 28, 2023

Coverage Status

coverage: 93.856% (-0.1%) from 93.953% when pulling bea2050 on mraspaud:fix-frequency into 2a6d769 on pytroll:main.

"""Return the bounding box lons and lats.

Args:
frequency:
bbox_shape (formerly frequency):
Copy link
Member

Choose a reason for hiding this comment

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

shape? Also I don't think you can put a parenthetical in the docstring like this as sphinx will treat it like a type declaration. Also shape usually refers to a numpy shape with (row, column). Should this rename maybe reflect that this is a number of points? I got an email that @ghiggi commented the same thing, but I don't see it on here anymore. num_points?

@@ -296,11 +296,12 @@ def get_boundary_lonlats(self):
return (SimpleBoundary(s1_lon.squeeze(), s2_lon.squeeze(), s3_lon.squeeze(), s4_lon.squeeze()),
SimpleBoundary(s1_lat.squeeze(), s2_lat.squeeze(), s3_lat.squeeze(), s4_lat.squeeze()))

def get_bbox_lonlats(self, frequency: Optional[int] = None, force_clockwise: bool = True) -> tuple:
def get_bbox_lonlats(self, bbox_shape: Optional[int] = None, force_clockwise: bool = True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use n_vertices, n_vertices_per_side (preferred) or something like that? Using "shape" makes me think to an array shape ... or the output shape...

In this case the length of lons/lats is 4*bbox_shape right?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you switch to n_vertices_per_side I commit to do the PR next week to enable tuple arguments (of size 2 or 4) :-P

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree with @ghiggi here, any comments to that suggestion @mraspaud ?

Copy link
Contributor

@adybbroe adybbroe left a comment

Choose a reason for hiding this comment

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

LGTM except for the choice of parameter name.

@ghiggi
Copy link
Contributor

ghiggi commented Jul 6, 2023

Hey @mraspaud :) If you manage to get this merged in the next couple of days, next week I can make the PRs for what was discussed in #525 ;)

@mraspaud
Copy link
Member Author

mraspaud commented Jul 7, 2023

Ok, I renamed to vertices_per_side and adjusted the docstrings to remove the parenthesis

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.

One change and one other suggestion that I'm curious about.

pyresample/geometry.py Outdated Show resolved Hide resolved
pyresample/geometry.py Outdated Show resolved Hide resolved
@djhoese
Copy link
Member

djhoese commented Aug 14, 2023

Coverage is reporting that the decimate method in the boundary objects is not being used anymore as well as AreaDefBoundary, but I don't see why the changes in this PR specifically are causing that. Any ideas?

https://coveralls.io/builds/61960626/source?filename=pyresample%2Fboundary.py#L106

@mraspaud
Copy link
Member Author

I can't see anything in the PR that would change the coverage...

@djhoese
Copy link
Member

djhoese commented Aug 15, 2023

I looked at the results again and I think there may be some confusion in the coverage reports about which commit is being reported on or something. There are coverage changes that seem relatively unrelated to what's being done here. Let's merge this and see what happens?

@djhoese djhoese merged commit 04ea38d into pytroll:main Aug 15, 2023
23 of 24 checks passed
@mraspaud mraspaud deleted the fix-frequency branch August 16, 2023 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants