Fix spill over of ocean/and features in cartopy plots in case of geostationary full disc plot. #601
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #601 +/- ##
==========================================
- Coverage 94.01% 93.97% -0.04%
==========================================
Files 92 86 -6
Lines 13836 13811 -25
==========================================
- Hits 13008 12979 -29
- 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/utils/cartopy.py
Outdated
|
|
||
| # For geostationary full disc the boundary needs to be the actuall boundary of the earth disc otherwise | ||
| # when ocean or land features are added to the cartopy plot these spill over. | ||
| if "Geostationary Satellite" in crs.to_wkt(): |
There was a problem hiding this comment.
It would be great if this if block for geos was put into a separate function/method. Or at least invert the condition and put the much smaller else: block in the main if block and return immediately.
| @@ -69,3 +70,70 @@ def __init__(self, | |||
| if self.bounds is not None: | |||
| x0, x1, y0, y1 = self.bounds | |||
| self.threshold = min(abs(x1 - x0), abs(y1 - y0)) / 100. | |||
|
|
|||
| # For geostationary full disc the boundary needs to be the actuall boundary of the earth disc otherwise | |||
There was a problem hiding this comment.
What if it's only part of the disc, but including some boundary?
There was a problem hiding this comment.
I think this gets into the difference between how I thought and wanted the cartopy projection classes to work (as an AreaDefinition clone) versus how they are intended to work. In all my usage and examples of this class I use it to hold the projection information but also the literal bounds of the data. Cartopy uses bounds to mean the actual physical (?) bounds of the projection space, not of the data. Cartopy/matplotlib use xlims/ylims for data bounds.
So I wonder if we could take the produced geostationary bound here and clip it to the x and y limits passed by the user. I'm not sure that is always accurate though.
Or...we completely rewrite these interfaces and/or examples so that they use upstream cartopy projection objects and includes an extra step to set the xlim/ylims of the plot to match the data.
There was a problem hiding this comment.
This is exactly the issue. And while I think it makes sense to rethink this for the display of the html representation of the AreaDefinition this currently seems to work.
There was a problem hiding this comment.
Ok @BENR0, but doesn't the current method (_geos_boundary) get the full disk geostationary boundary? So if I try to do the HTML representations of a subset of the full disk (ex. ABI CONUS or mesoscale sector) then my representation would be much bigger than intended/expected?
| @property | ||
| def boundary(self): | ||
| """Return boundary.""" | ||
| return self._boundary |
There was a problem hiding this comment.
I'm OK merging this if it improves what you're seeing and trying to fix. I still have a question on non-full disk geostationary images and what they look like with (or without) this PR. I made one other comment about if the boundary property is strictly needed, but that's not a big deal.
@mraspaud if you wake up and think this looks fine as is then feel free to merge and make a release. If this is not ready then I'd be fine with a release of main as is and save this for a bug fix release shortly after.
|
Yes this should fix the problem with the geostationary plots. I am on vacation this week and can not make plots and post them here. I would also be fine with merging this later if you want to make a release this week. |
|
Ok I've made a couple changes and tested things in a jupyter notebook. It looks like ABI CONUS shows as expected which was my main concern. First I added a context manager to remove the PROJ warnings when getting the PROJ dict. The context manager was created by me for cases exactly like this where we know that PROJ.4 is inexact, but it is the representation we want to use/display. The other fix was converting extent floats to |
|
I'll try to make a release including this PR later tonight. Have to go for now. |
In case of geostationary full disc plots the boundary of the cartopy CRS needs to be the actual full disc otherwise ocean/land features spill over.
Since the
to_cartopy_crsmethod in pyresample uses a custom Projection class this is a hot fix using code from the CartopyGeostationaryclass.