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

More flexible way of passing avhrr_l1b_gaclac reader kwargs to pygac #1237

Merged
merged 5 commits into from
Jul 9, 2020

Conversation

sfinkens
Copy link
Member

@sfinkens sfinkens commented Jun 15, 2020

Some reader arguments in avhrr_l1b_gaclac are directly passed to the corresponding pygac reader, and not used further in the satpy reader. To make the reader more flexible (and to prevent having to change satpy each time the reader signature in pygac changes), collect transitional arguments in **reader_kwargs and pass them to pygac. It doesn't obfuscate the call, because pygac will complain about invalid keyword arguments.

Furthermore, remove handling of derived metadata like midnight scanline and missing scanlines. They can easily be computed by users (via timestamps and qual flags) and so we have less work maintaining them.

  • Tests added
  • Tests passed
  • Passes flake8 satpy

@sfinkens sfinkens self-assigned this Jun 15, 2020
@sfinkens sfinkens added component:readers enhancement code enhancements, features, improvements labels Jun 15, 2020
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0003%) to 90.022% when pulling 1d23dd8 on sfinkens:pygac-metadata into 4b92a3c on pytroll:master.

@coveralls
Copy link

coveralls commented Jun 15, 2020

Coverage Status

Coverage increased (+0.02%) to 90.04% when pulling e9997f7 on sfinkens:pygac-metadata into 4b92a3c on pytroll:master.

@codecov
Copy link

codecov bot commented Jun 15, 2020

Codecov Report

Merging #1237 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1237      +/-   ##
==========================================
+ Coverage   90.02%   90.03%   +0.01%     
==========================================
  Files         218      218              
  Lines       31259    31275      +16     
==========================================
+ Hits        28141    28160      +19     
+ Misses       3118     3115       -3     
Impacted Files Coverage Δ
satpy/readers/avhrr_l1b_gaclac.py 95.03% <100.00%> (+1.69%) ⬆️
satpy/tests/reader_tests/test_avhrr_l1b_gaclac.py 99.58% <100.00%> (+0.01%) ⬆️
satpy/composites/__init__.py 80.70% <0.00%> (ø)
satpy/tests/reader_tests/test_agri_l1.py 100.00% <0.00%> (ø)
satpy/tests/test_composites.py 99.88% <0.00%> (+<0.01%) ⬆️

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 4b92a3c...e9997f7. Read the comment docs.

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.

Just a comment for now

satpy/readers/avhrr_l1b_gaclac.py Outdated Show resolved Hide resolved
@sfinkens sfinkens changed the title Add calibration argument to avhrr_l1b_gaclac reader More flexible way of passing avhrr_l1b_gaclac reader kwargs to pygac Jun 23, 2020
@sfinkens sfinkens requested a review from adybbroe June 23, 2020 17:10
@sfinkens
Copy link
Member Author

@ninahakansson If you have time, I'd be pleased if you could review this PR

@sfinkens sfinkens marked this pull request as ready for review June 23, 2020 17:12
@sfinkens sfinkens requested a review from djhoese as a code owner June 23, 2020 17:12
Copy link
Contributor

@ninahakansson ninahakansson 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! I like the more flexible use of kwargs. I tested this with
level1c4pps and it works fine.

I see that missing_scanlines are not any longer updated when data are sliced.
I am not sure if it could be a problem. But I agree that is is easier to not have to maintain
it inside satpy.

@sfinkens
Copy link
Member Author

@ninahakansson Thanks for taking the time! Yes, that's true, at least for the moment. My plan is to remove the missing/midnight scanline attributes from pygac, too. In my opinion it makes more sense that the user computes them if needed. But we can certainly provide a method to compute that in pygac.

@mraspaud
Copy link
Member

mraspaud commented Jul 9, 2020

LGTM. Is this ready to merge @sfinkens ?

@sfinkens
Copy link
Member Author

sfinkens commented Jul 9, 2020

@mraspaud Yes please! 🙂

@mraspaud mraspaud merged commit 3128bf5 into pytroll:master Jul 9, 2020
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

4 participants