-
Notifications
You must be signed in to change notification settings - Fork 293
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
Check ancillary_variables in exclude_attrs before get_extra_ds #1140
Conversation
Congratulations 🎉. DeepCode analyzed your code in 0.238 seconds and we found no issues. Enjoy a moment of no bugs ☀️. 👉 View analysis in DeepCode’s DashboardEdit DeepCode’s Configurations here |
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.
I'm not sure this makes sense. extra
will always be false if there are ancillary_variables
which means that that for loop will never do anything. You've essentially just dropped the ancillary_variables
attribute haven't you? Additionally, now these ancillary variables won't be included in the NetCDF file but we want them to be.
In my opinion this is both a bug in the tropomi data (does it even make scientific sense for the circular definition?) and satpy which could/should probably defend against this circular dependency.
Perhaps the "right" fix would be to include the currently processed dictionary to get_extra_ds
so that it knows what it has already analyzed instead of doing it multiple times.
@djhoese Nice idea to let BTW, I would ask someone in KNMI about the "wrong" |
Codecov Report
@@ Coverage Diff @@
## master #1140 +/- ##
==========================================
+ Coverage 89.60% 89.95% +0.35%
==========================================
Files 200 208 +8
Lines 29484 30605 +1121
==========================================
+ Hits 26420 27532 +1112
- Misses 3064 3073 +9
Continue to review full report at Codecov.
|
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.
Nice catch, thanks for the pr. Could you add a small test for this?
scn.save_datasets(filename=filename, writer='cf') | ||
with xr.open_dataset(filename) as f: | ||
self.assertEqual(f['test-array-1'].attrs['ancillary_variables'], | ||
'test-array-2') |
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.
As mentioned in the issue in xarray, ancillary_variables
isn't decoded.
So, since save_datasets
succeeds, I just check the name here.
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.
Just a small suggestion (docstring style), otherwise looks good to me.
I suppose this PR is fine enough for checking ancillary_variables. |
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.
Looks good to me. Thanks.
I found this problem when I was trying to save tropomi
scene
tonetcdf
file.There're two variables cited each other by
ancillary_variables
:I don't know this should be the bug of
tropomi_l2
file or this is correct. This could cause the infinite recursion ofget_extra_ds
., like this:Anyway, I decide to add
ancillary_variables
toexclude_attrs
when usingsave_datasets
for this issue. So, we need to check the exist ofancillary_variables
beforeget_extra_ds
.