Fix VIIRS EDR using the wrong geolocation arrays #2842
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2842 +/- ##
=======================================
Coverage 95.94% 95.94%
=======================================
Files 366 366
Lines 53561 53584 +23
=======================================
+ Hits 51389 51412 +23
Misses 2172 2172
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| # "standard" geolocation coordinate (assume shorter variable name is "better") | ||
| new_name = (m_lon_name if res == 750 else i_lon_name) if is_lon else ( | ||
| m_lat_name if res == 750 else i_lat_name) | ||
| if new_name not in coords or len(var_name) < len(coords[new_name]["file_key"]): |
There was a problem hiding this comment.
Would it be better to check explicitly for the parallax correction in the variable name? Or are there multiple of these that have "longitude" in the name? This name-length check would fail if they decided to add lon and lat as the "normal" coordinate arrays and the reader would need to adapted for them.
There was a problem hiding this comment.
Good question. You're right that the more explicit/specific solution would be to just check for the exact case I'm solving for. However, these parallax correction versions only show up (as far as I can find) in the "CloudHeight" files. Since this reader is supposed to be generic and potentially able to handle any VIIRS EDR file I'm worried about a hardcoded solution like that being not useful for these cases.
Side note: The SurfRefl files have multiple lon/lat arrays for the different resolutions of VIIRS, but I had to do some other customizations for these data files so they are hardcoded in the YAML.
Pull Request Test Coverage Report for Build 9700068663Details
💛 - Coveralls |
|
Is there a way for the user to select which one is used? I can think of many circumstances in which the parallax corrected versions would be preferred (despite the metadata issues). |
|
@simonrp84 At the moment, no. It would make sense, but maybe we save that for another PR if someone really wants it. Right now, besides this cloud height file, there are no other lon/lats available in the EDR files that I'm familiar with. We could either do a kwarg-style choice for users or there is (technically) the ability to customize the YAML right now and describe all the products you want and their specific lon/lats. |
|
I got the "good enough" signal from Panu on slack. Merging...thanks for the reviews. |
Pull Request Test Coverage Report for Build 9702660698Details
💛 - Coveralls |
The VIIRS EDR CloudHeight files includes not only normal lon/lat arrays but a parallax corrected lon/lat. These parallax lon/lats don't include proper metadata (ex.
_FillValue) so loading them causes fill values to remain like-999.9instead of NaN. My previous code in this reader was rather naive and basically said if a variable name contained "longitude" or "latitude" then we should use it as the geolocation for our data arrays. This code was picking up the parallax corrected lon/lats instead of the "standard" ones. This PR is an attempt at making the reader smarter at choosing the right lon/lat arrays.CC @kathys
AUTHORS.mdif not there already