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

Add access point to global_attrs to netCDF4FileHandler #1772

Merged
merged 13 commits into from Aug 20, 2021
Merged

Add access point to global_attrs to netCDF4FileHandler #1772

merged 13 commits into from Aug 20, 2021

Conversation

tsukada-cs
Copy link
Contributor

@tsukada-cs tsukada-cs commented Jul 23, 2021

In the NetCDF4FileHandler, it can't get all global_attributes in NetCDF with a single command.
I think NetCDF4FileHandler should have a method to get all global_attributes at once without the attribute’s name.

In this PR, we can access all global_attributes with

wrapper['/attr'] or wrapper['/attrs']

  • Tests added
  • Fully documented

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

This looks good to me, just a linting error to fix so far :)

satpy/tests/reader_tests/test_netcdf_utils.py Outdated Show resolved Hide resolved
Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>
@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #1772 (0dcfd46) into main (4d34625) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1772      +/-   ##
==========================================
+ Coverage   92.82%   92.91%   +0.08%     
==========================================
  Files         263      265       +2     
  Lines       38717    39037     +320     
==========================================
+ Hits        35940    36272     +332     
+ Misses       2777     2765      -12     
Flag Coverage Δ
behaviourtests 4.76% <0.00%> (-0.04%) ⬇️
unittests 93.45% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
satpy/readers/netcdf_utils.py 100.00% <100.00%> (ø)
satpy/tests/reader_tests/test_netcdf_utils.py 94.89% <100.00%> (+0.10%) ⬆️
satpy/readers/abi_l1b.py 95.83% <0.00%> (-0.89%) ⬇️
satpy/scene.py 92.32% <0.00%> (ø)
satpy/_config.py 98.59% <0.00%> (ø)
satpy/demo/__init__.py 100.00% <0.00%> (ø)
satpy/demo/viirs_sdr.py 100.00% <0.00%> (ø)
satpy/readers/abi_base.py 94.59% <0.00%> (ø)
satpy/tests/reader_tests/test_clavrx.py 100.00% <0.00%> (ø)
satpy/tests/reader_tests/test_abi_l1b.py 100.00% <0.00%> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d34625...0dcfd46. Read the comment docs.

@coveralls
Copy link

coveralls commented Jul 23, 2021

Coverage Status

Coverage increased (+0.04%) to 93.398% when pulling 0dcfd46 on tsukada-cs:feature-add_access_point_to_global_attrs_to_NetCDF4FileHandler into 07a87af on pytroll:main.

satpy/readers/netcdf_utils.py Outdated Show resolved Hide resolved
satpy/readers/netcdf_utils.py Outdated Show resolved Hide resolved
satpy/readers/netcdf_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Ok, I think these are my last comments, after that it should be good to merge :)

satpy/readers/netcdf_utils.py Outdated Show resolved Hide resolved
satpy/readers/netcdf_utils.py Show resolved Hide resolved
satpy/readers/netcdf_utils.py Show resolved Hide resolved
satpy/readers/netcdf_utils.py Outdated Show resolved Hide resolved
tsukada-cs and others added 4 commits July 23, 2021 17:37
Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>
Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>
Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>
Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>
@tsukada-cs
Copy link
Contributor Author

Thanks a lot!

Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM

@mraspaud mraspaud added component:readers enhancement code enhancements, features, improvements labels Jul 23, 2021
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Nice work! One more consideration: what do you think about storing the global attributes in a separate dictionary (ex. self._global_attrs = {}) or as a separate key (ex. self.file_content['/attrs'] = {}) and have that filled in in the _collect_attrs method when the file is read. We're already storing all the global attributes in self.file_content so having another copy wouldn't be that bad. You could have a special _collect_global_attrs method to do it maybe. Just a thought.

@tsukada-cs
Copy link
Contributor Author

That's also nice idea! But I'd like to see what's going on with the current implementation. Thank you :)

@djhoese
Copy link
Member

djhoese commented Jul 25, 2021

But I'd like to see what's going on with the current implementation.

I'm not sure I understand what you mean. Are you saying you like the current implementation and don't want to change it?

@tsukada-cs
Copy link
Contributor Author

@djhoese Sorry for the confusion. I don't really have time for this. If someone wants to change the implementation, that's fine.

@djhoese
Copy link
Member

djhoese commented Aug 18, 2021

@mraspaud I'm working on refactoring this and I'd like to remove the ability to use /attr and /attrs and instead make it only /attrs. Are you and @tsukada-cs ok with that?

Additional question: Should the globals dictionary have attribute keys with /attr/ prefixes? Or just the straight key?

self["/attrs"]["some_key"]
# versus
self["/attrs"]["/attr/some_key"]

@tsukada-cs
Copy link
Contributor Author

@djhoese

Are you and @tsukada-cs ok with that?

Thank you. There is no problem with that.

I prefer

self["/attrs"]["some_key"]

, but I have no strong preference or reason for it.

@mraspaud
Copy link
Member

Sounds good. I agree that self["/attrs"]["some_key"] looks better.

@djhoese djhoese requested a review from mraspaud August 19, 2021 13:13
@djhoese
Copy link
Member

djhoese commented Aug 19, 2021

Ok this is ready for re-review. Global attributes are available as handle["/attrs"] and the keys in that dictionary are just the name of the attribute. I also refactored some of the other code to hopefully clean it up a bit (smaller methods, less redundant code).

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM, it's nice with the simplification of the interface

@mraspaud mraspaud merged commit a6a3a66 into pytroll:main Aug 20, 2021
@tsukada-cs tsukada-cs deleted the feature-add_access_point_to_global_attrs_to_NetCDF4FileHandler branch August 20, 2021 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:readers enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants