Fix HDF4 support in geocat reader with hardcoded engine #2507

joleenf · 2023-06-12T16:22:58Z

Adds an __init__ method to the GEOCATFileHandler to allow the addition of xarray_kwargs to the reader. This has been done so that the engine to the xarray_opendataset can be set as well as setting decode_times=False since the current metadata does not allow xarray to decode the times. (Even when the engine is set, decode_times=True will throw an error because xarray cannot interpret the units metadata as written by GEOCAT).

Closes Add xarray_kwargs capability to the geocat reader #2506
Tests added: A basic test that adds an xarray_kwarg and checks to see that datasets have been loaded with the reader.

Relates to the discussion on pydata/xarray#7905
I wanted to isolate this pull request from later work which will rely on being able to read these GEOCAT files with SatPy (i.e. adding aliases to facilitate RGB composites with existing recipes).

…dler that using xarray_kwargs.

djhoese · 2023-06-12T16:29:35Z

satpy/readers/geocat.py

+        scene = satpy.Scene(filenames,
+                            reader='geocat',
+                            reader_kwargs={'decode_times': True})


Correct me if I'm wrong but this isn't how this would work. The user would be doing reader_kwargs={"xarray_kwargs": {"decode_times": True}}.

You are correct fixed in 510c24f

djhoese · 2023-06-12T16:30:35Z

satpy/readers/geocat.py

+        kwargs.setdefault('xarray_kwargs', {}).setdefault(
+            'decode_times', False)
+        kwargs.setdefault('xarray_kwargs', {}).setdefault(
+            'engine', "netcdf4")


Thinking about it, these can just be hardcoded can't they? So kwargs.setdefault("xarray_kwargs", {"decode_times": False, "engine": "netcdf4"})?

At this point, I can't think of a use case where they could be anything else.

If the geocat code changes, and the unit metadata becomes compliant, I think it would be nice to be able to interpret the scan line times when reading them. I might be mistaken, but 75-78 as written would allow the user to set the decode_times back to True if it is possible to decode them?

Yeah, it looks like it would work. My suggestion for doing the entire xarray_kwargs dictionary would also work, but would require users to make sure they include engine and decode_times if they were specifying xarray_kwargs at all. This is such a small thing and not sure it matters too much.

Okay. I will work on it. However, if I set as you outline above, and the user enters:

a = Scene([fn], reader="geocat", reader_kwargs={"xarray_kwargs": {"decode_times": True}})

That will strip the default engine so that what is passed to xarray.open_dataset is decode_times=True, but no engine. As I am understanding what you are saying, and I think it is necessary, the reader should still pass engine="netcdf4" regardless of what the call from Scene contains. Correct?

Circling back to this again, sorry.... if the kwargs are set as in a previous commit

Doesn't that leave the engine as "netcdf4" if the user only enters a value for decode_times? If no xarray_kwargs are provided, the values would be {"engine": "netcdf4", "decode_times": False}?

I think I am not understanding your point fully on this requested change to
kwargs.setdefault("xarray_kwargs", {"decode_times": False, "engine": "netcdf4"}).

You are correct. Yours is more flexible. That's why I said it doesn't matter much. My main reason for my suggestion was that it treated xarray_kwargs as an all-or-nothing keyword argument rather than individual things. Sorry for all the confusion for something so dumb (on my part). Let me know if you want to put it back the way you had it or leave it as is. Either way I'm fine with it and we can merge regardless of the pre-commit issue...which I think is a hiccup on pre-commit's side of things.

It is fine. I think either way works, so to avoid errors that can be introduced with too much more fiddling, I think I will leave it as is.

codecov · 2023-06-12T16:47:24Z

Codecov Report

Merging #2507 (bc8a955) into main (9415ad9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2507   +/-   ##
=======================================
  Coverage   94.85%   94.85%           
=======================================
  Files         337      337           
  Lines       49624    49635   +11     
=======================================
+ Hits        47069    47080   +11     
  Misses       2555     2555

Flag	Coverage Δ
behaviourtests	`4.41% <0.00%> (-0.01%)`	⬇️
unittests	`95.47% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
satpy/readers/geocat.py	`88.81% <100.00%> (+0.28%)`	⬆️
satpy/tests/reader_tests/test_geocat.py	`100.00% <100.00%> (ø)`

coveralls · 2023-06-12T17:06:16Z

Pull Request Test Coverage Report for Build 5315827401

13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.001%) to 95.42%

Totals
Change from base Build 5268800416:	0.001%
Covered Lines:	47193
Relevant Lines:	49458

💛 - Coveralls

djhoese · 2023-06-12T21:01:32Z

satpy/readers/geocat.py

+        """Open and perform initial investigation of NetCDF file."""
+        super(GEOCATFileHandler, self).__init__(
+            filename, filename_info, filetype_info,
+            xarray_kwargs={"engine": "netcdf4", "decode_times": False})


I guess the only change I'd like to consider is allowing the user to pass xarray_kwargs in a **kwargs. I think we had done something like kwargs.setdefault("xarray_kwargs, {...}).

djhoese · 2023-06-14T18:56:38Z

satpy/readers/geocat.py

+        super(GEOCATFileHandler, self).__init__(
+            filename, filename_info, filetype_info,
+            xarray_kwargs=kwargs["xarray_kwargs"])


I think this should just be **kwargs in case the user is passing other kwargs down to the utility file handler.

I think I understand where you want kwargs, in the __init__ correct? The user would still send the xarray specific kwargs with the key "xarray_kwargs" so the documentation should reflect that.

Correct. This call to __init__ should use kwargs to pass everything to the base class. That doesn't change what the user uses.

satpy/readers/geocat.py

djhoese · 2023-06-14T18:57:02Z

satpy/readers/geocat.py

+
+    **Loading data with decode_times=True**
+
+    By default, this reader will use xarray_kwargs={"engine": "netcdf4", "decode_times": False}.


Suggested change

By default, this reader will use xarray_kwargs={"engine": "netcdf4", "decode_times": False}.

By default, this reader will use ``xarray_kwargs={"engine": "netcdf4", "decode_times": False}``.

Fix the documentation reference for the Scene class. Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>

djhoese · 2023-06-15T20:59:48Z

satpy/readers/geocat.py

+        scene = satpy.Scene(filenames,
+                            reader='geocat',
+                            reader_kwargs={'xarray_kwargs': {'decode_times': True}})


Do you want to update this to include engine="netcdf" in this example since without it and with my proposed code that you implemented the engine will be the default where xarray tries to match/find one.

satpy/tests/reader_tests/test_geocat.py

Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>

djhoese · 2023-06-16T19:19:09Z

pre-commit.ci autofix

djhoese · 2023-06-19T02:02:44Z

Closing and reopening to hopefully pick up any commits from pre-commit.

djhoese · 2023-06-19T02:03:36Z

pre-commit.ci autofix

djhoese · 2023-06-19T02:06:56Z

@joleenf I checked the pre-commit.ci log and it looks like it is having some trouble merging/pushing fixes to your branch. It says to pull/merge with upstream main to fix it. You could also do the fixes yourself (after merging with main) by running pre-commit run -a and that should make the changes pre-commit.ci would have made anyway.

Merge branch 'main' of github.com:pytroll/satpy into geocat_xr_kwargs

user does not have to enter both the engine and decode_times when changing one of the defaults.

joleenf added 4 commits March 10, 2023 14:32

Merge branch 'main' of github.com:pytroll/satpy into main

6f141e8

Merge branch 'main' of github.com:pytroll/satpy into main

8baf5af

Merge branch 'main' of github.com:pytroll/satpy into main

b301a89

Address Issue pytroll#2506 by adding an __init__ to the GEOCATFileHan…

b8a42de

…dler that using xarray_kwargs.

djhoese reviewed Jun 12, 2023

View reviewed changes

joleenf force-pushed the geocat_xr_kwargs branch from 78ed846 to b8a42de Compare June 13, 2023 11:11

Fix documenation about reader_kwargs

510c24f

djhoese reviewed Jun 14, 2023

View reviewed changes

satpy/readers/geocat.py Outdated Show resolved Hide resolved

djhoese reviewed Jun 14, 2023

View reviewed changes

joleenf and others added 3 commits June 15, 2023 08:46

Update satpy/readers/geocat.py

24415b1

Fix the documentation reference for the Scene class. Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>

Fix quoting around xarray_kwargs documentation

8f71da4

Set defaults to kwargs so that they are netCDF4 and decode_times=False

6edfb35

joleenf marked this pull request as ready for review June 15, 2023 20:48

joleenf requested a review from mraspaud as a code owner June 15, 2023 20:48

joleenf requested a review from djhoese June 15, 2023 20:48

djhoese reviewed Jun 15, 2023

View reviewed changes

satpy/tests/reader_tests/test_geocat.py Outdated Show resolved Hide resolved

djhoese added bug component:readers labels Jun 16, 2023

djhoese self-assigned this Jun 16, 2023

joleenf and others added 2 commits June 16, 2023 11:53

Update satpy/tests/reader_tests/test_geocat.py

8b8e3e9

Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>

Just update the test __init__ asserts as well.

ca6a986

djhoese closed this Jun 19, 2023

djhoese reopened this Jun 19, 2023

joleenf added 2 commits June 19, 2023 09:47

Need to merge to fix pre-commit

08fbddd

Merge branch 'main' of github.com:pytroll/satpy into geocat_xr_kwargs

Adding file that pre-commit changed

a8ee418

joleenf requested a review from adybbroe as a code owner June 19, 2023 15:49

Revert back to individual setdefaults for xarray_kwargs so that

bc8a955

user does not have to enter both the engine and decode_times when changing one of the defaults.

joleenf requested a review from djhoese June 19, 2023 20:54

djhoese approved these changes Jun 20, 2023

View reviewed changes

djhoese changed the title ~~Add reader keyword argument xarray_kwargs~~ Add xarray_kwargs to geocat reader Jun 20, 2023

djhoese changed the title ~~Add xarray_kwargs to geocat reader~~ Fix HDF4 support in geocat reader with hardcoded engine Jun 20, 2023

djhoese merged commit 1d37638 into pytroll:main Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HDF4 support in geocat reader with hardcoded engine #2507

Fix HDF4 support in geocat reader with hardcoded engine #2507

joleenf commented Jun 12, 2023

djhoese Jun 12, 2023

joleenf Jun 14, 2023 •

edited

Loading

djhoese Jun 12, 2023

joleenf Jun 12, 2023

joleenf Jun 15, 2023

djhoese Jun 15, 2023

joleenf Jun 15, 2023 •

edited

Loading

joleenf Jun 16, 2023

djhoese Jun 16, 2023

joleenf Jun 16, 2023

codecov bot commented Jun 12, 2023 •

edited

Loading

coveralls commented Jun 12, 2023 •

edited

Loading

djhoese Jun 12, 2023

djhoese Jun 14, 2023

joleenf Jun 17, 2023

djhoese Jun 17, 2023

djhoese Jun 14, 2023

djhoese Jun 15, 2023

djhoese commented Jun 16, 2023

djhoese commented Jun 19, 2023

djhoese commented Jun 19, 2023

djhoese commented Jun 19, 2023


		Loading data with decode_times=True

		By default, this reader will use xarray_kwargs={"engine": "netcdf4", "decode_times": False}.

	By default, this reader will use xarray_kwargs={"engine": "netcdf4", "decode_times": False}.
	By default, this reader will use ``xarray_kwargs={"engine": "netcdf4", "decode_times": False}``.

Fix HDF4 support in geocat reader with hardcoded engine #2507

Fix HDF4 support in geocat reader with hardcoded engine #2507

Conversation

joleenf commented Jun 12, 2023

Choose a reason for hiding this comment

joleenf Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joleenf Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 12, 2023 • edited Loading

Codecov Report

coveralls commented Jun 12, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5315827401

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djhoese commented Jun 16, 2023

djhoese commented Jun 19, 2023

djhoese commented Jun 19, 2023

djhoese commented Jun 19, 2023

joleenf Jun 14, 2023 •

edited

Loading

joleenf Jun 15, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading

coveralls commented Jun 12, 2023 •

edited

Loading