-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fix SwathDefinition html representation error when lons/lats 1D #610
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #610 +/- ##
==========================================
- Coverage 93.98% 93.96% -0.03%
==========================================
Files 86 86
Lines 13768 13786 +18
==========================================
+ Hits 12940 12954 +14
- Misses 828 832 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pyresample/_formatting_html.py
Outdated
resolution_str = "{resolution}/{resolution}".format(resolution="Na") | ||
height, width = "Na", "Na" |
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.
How about all caps:
resolution_str = "{resolution}/{resolution}".format(resolution="Na") | |
height, width = "Na", "Na" | |
resolution_str = "{resolution}/{resolution}".format(resolution="N/A") | |
height, width = "N/A", "N/A" |
Or maybe "NA"? For the resolution string, why not just hardcode "NA/NA" instead of formatting? Otherwise, should resolution use a different separator than /
? Maybe x
?
Not sure why I didn't ask this before, but what is 40075000?
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.
Yeah you are right, I will just hard code it. I just took the code from the other cases but does not make sense here. Good idea with the "x" instead of "/" definitely makes more sense.
Well as mentioned somewhere before the resolution calculation in case of the attributes not having resolution set is just a rough estimate and therefore the circumference of the earth (40075000) is just hard coded.
pyresample/_formatting_html.py
Outdated
area_units = "m" | ||
else: | ||
lon_attrs = area.lons.attrs | ||
lat_attrs = area.lats.attrs | ||
if isinstance(area.lons, np.ndarray) & isinstance(area.lats, np.ndarray): |
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.
if isinstance(area.lons, np.ndarray) & isinstance(area.lats, np.ndarray): | |
if isinstance(area.lons, np.ndarray) and isinstance(area.lats, np.ndarray): |
Looks like the unstable environment are failing for some related things, but not only related things. I wonder if this is something we could/should fix in this PR:
|
Bug report for the failures: shapely/shapely#2098 Let's merge this since these failures are not your fault and are in the unstable environment. |
This is a quick fix for #609. It just sets things like resolution and width/height to Na in the case of 1D lat/lon arrays so the html representation does not break.
It also does some quick fixing of formatting issues I noticed.
git diff origin/main **/*py | flake8 --diff