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

Fix issues with unwrapping longitudes in RangeXY stream #756

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

philippjfr
Copy link
Member

This PR introduced two separate issues: #722

Fixes #753

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.28%. Comparing base (5f02c71) to head (eea3ee9).

Files with missing lines Patch % Lines
geoviews/plotting/bokeh/callbacks.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #756      +/-   ##
==========================================
+ Coverage   67.21%   67.28%   +0.07%     
==========================================
  Files          44       44              
  Lines        4642     4649       +7     
==========================================
+ Hits         3120     3128       +8     
+ Misses       1522     1521       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ahuang11
Copy link
Collaborator

ahuang11 commented Oct 16, 2024

Something is still up in this PR
image

!wget -nc https://downloads.psl.noaa.gov/Datasets/ncep.reanalysis/Monthlies/surface/air.day.ltm.nc
import xarray as xr
import holoviews as hv
from holoviews.operation.datashader import rasterize
import geoviews as gv
gv.extension("bokeh")

ds = xr.open_dataset("air.day.ltm.nc", engine="h5netcdf", decode_times=False).isel(time=0)
rasterize((gv.Image(ds["air"], ["lon", "lat"], ["air"]))) * gv.feature.coastline()

@ahuang11
Copy link
Collaborator

ahuang11 commented Oct 16, 2024

I'm starting to think #722 wasn't the right solution because if the data extents are global from 0 to 360, the ranges wrap to -180 to 180, meaning only 0 to 180 remains while 180-360 is cropped and discarded, resulting in half image:
image

I think we need to either warn and project the lons to -180 to 180 or raise an error mentioning that data must be from -180 to 180 for operations to work properly, i.e.

ds["lon"] = (ds["lon"] + 180) % 360 - 180
ds = ds.sortby("lon")

Separately, there seems to be some padding with the x0/x1:


            self._unwrap_lons = 0 <= x0 <= 360 and 180 <= x1 <= 360
            print(x0, x1, "Xs", self._unwrap_lons)

# outputs
# -1.25 358.75 Xs False
# -1.25 358.75 Xs False

@ahuang11
Copy link
Collaborator

Actually, if we extend the range from -180 to 540, it seems to work as expected:

Screen.Recording.2024-10-16.at.10.38.58.AM.mov
Screen.Recording.2024-10-16.at.10.45.50.AM.mov

@ahuang11 ahuang11 requested a review from hoxbro October 24, 2024 18:25
Copy link
Member

@hoxbro hoxbro left a comment

Choose a reason for hiding this comment

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

Can you add test cases for all the cases you have tested locally so we don't get any regressions?

If needed, I can set up UI tests.

@ahuang11
Copy link
Collaborator

Yes can you help me set up ui tests or show me a reference?

1 similar comment
@ahuang11
Copy link
Collaborator

Yes can you help me set up ui tests or show me a reference?

@hoxbro
Copy link
Member

hoxbro commented Oct 28, 2024

I have opened a PR with the infrastructure here: #760.

I will also try to fix the CI in it and then merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: RangeXY now provides incorrect longitude x_range
3 participants