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

Fix nucaps reader failing when kwargs are passed #1303

Merged
merged 2 commits into from Sep 9, 2020

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Aug 7, 2020

Polar2Grid can't use the nucaps reader with semi-recent versions of Satpy because it always passes the mask_surface and mask_quality flags to the readers and the file handler blindly passed them on to its base classes. Instead, these were supposed to be provided to the reader and ignored by the file handler.

I think this is a good example of how the tests are maybe a little too low-level and should be using higher level interfaces for testing (creating readers).

  • Closes #xxxx
  • Tests added
  • Tests passed
  • Passes flake8 satpy

@coveralls
Copy link

coveralls commented Aug 7, 2020

Coverage Status

Coverage decreased (-0.002%) to 90.015% when pulling 1b47726 on djhoese:bugfix-nucaps-kwargs into 4c11f8b on pytroll:master.

@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #1303 into master will increase coverage by 0.00%.
The diff coverage is 93.75%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1303   +/-   ##
=======================================
  Coverage   90.01%   90.01%           
=======================================
  Files         220      220           
  Lines       32244    32258   +14     
=======================================
+ Hits        29025    29038   +13     
- Misses       3219     3220    +1     
Impacted Files Coverage Δ
satpy/tests/reader_tests/test_netcdf_utils.py 94.79% <85.71%> (-0.82%) ⬇️
satpy/readers/nucaps.py 89.14% <100.00%> (+0.09%) ⬆️
satpy/tests/reader_tests/test_nucaps.py 100.00% <100.00%> (ø)

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 4c11f8b...1b47726. Read the comment docs.

@mraspaud
Copy link
Member

mraspaud commented Aug 7, 2020

Are you saying p2g is passing the mask flags to the file handlers?

@djhoese
Copy link
Member Author

djhoese commented Aug 7, 2020

No, sorry, let me clarify. Polar2Grid is passing reader_kwargs to the Scene and the Scene (or rather the code in yaml_reader.py) is passing these kwargs to the Reader object and the file handler instances. It is broken for this reader (and possibly other readers) because it is one of the few cases where there is a custom Reader class using kwargs instead of the file handler classes where I think most reader developers are doing it.

@mraspaud
Copy link
Member

mraspaud commented Aug 7, 2020

OK, then I don't get how mask_* is passed to the file handlers, since the super init is called without them.

@mraspaud
Copy link
Member

mraspaud commented Aug 7, 2020

Anyway, the right to have it would be to suppress the kwargs at the reader level I think

@djhoese
Copy link
Member Author

djhoese commented Aug 7, 2020

It is outside the yaml_reader.py actually and this is functionality that I was concerned about when it was added. People wanted to be able to pass kwargs to file handlers where I had previously limited it to the reader class only. Here is where the kwargs are passed to file handlers:

https://github.com/pytroll/satpy/blob/master/satpy/readers/__init__.py#L767

Keep in mind though that the user shouldn't need to know what is a reader kwarg and what is a file handler kwarg. I suppose yaml reader could be updated to support this kind of thing but it gets a little iffy with it knowing what kwargs are what.

@mraspaud
Copy link
Member

mraspaud commented Aug 7, 2020

OK, I understand. Indeed this is not optimal, but the ability to pass kwargs to the file handlers is good I think. As you say though, a user shouldn't have to know which class is consuming the kwargs.

Ideally, the reader would pop out the kwargs it's consuming from the list, and leave the rest untouched, to be passed on to the file handlers, eg through a property of the reader.

@djhoese
Copy link
Member Author

djhoese commented Aug 7, 2020

Ideally, the reader would pop out the kwargs it's consuming from the list, and leave the rest untouched, to be passed on to the file handlers, eg through a property of the reader.

Agreed, but in the current implementation the Reader instance is out of the loop for what kwargs go to file handlers. Best case would be to override the create_filehandlers method and pull out the kwargs there. Otherwise, the best solution would be to move the functionality in the load_readers code to be inside the reader. This gets into decoupling the reader creation again though (currently 2-3 methods need to be called, it should be 1 init or 1 init + 1 method max).

@mraspaud mraspaud added this to the v0.23.0 milestone Sep 9, 2020
@mraspaud mraspaud merged commit 78da121 into pytroll:master Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants