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

Remove duplicate entries of required netcdf variables in FCI reader #2634

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

pnuu
Copy link
Member

@pnuu pnuu commented Nov 10, 2023

While testing things to speed-up FCI reader, I noticed there were some duplicate/triplicate entries in the FCI reader YAML listing the required netcdf variables. Each of these in themselves caused extra file access, so this also speeds up the reader a tiny bit.

@pnuu pnuu added the cleanup Code cleanup but otherwise no change in functionality label Nov 10, 2023
@pnuu pnuu self-assigned this Nov 10, 2023
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

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Merging #2634 (f3c4c79) into main (ee63577) will increase coverage by 0.02%.
Report is 18 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2634      +/-   ##
==========================================
+ Coverage   95.18%   95.20%   +0.02%     
==========================================
  Files         354      354              
  Lines       51316    51368      +52     
==========================================
+ Hits        48846    48906      +60     
+ Misses       2470     2462       -8     
Flag Coverage Δ
behaviourtests 4.24% <ø> (-0.01%) ⬇️
unittests 95.83% <ø> (+0.02%) ⬆️

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

see 14 files with indirect coverage changes

@pnuu
Copy link
Member Author

pnuu commented Nov 10, 2023

There could be some others that are read twice even if not listed, like data/mtg_geos_projection bringing in all its attributes which are listed also separately. I'll see if this is true and clean also these.

@pnuu
Copy link
Member Author

pnuu commented Nov 10, 2023

Yep, all the separately listed attributes of data/mtg_geos_projection were already read when collecting the parent.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6822460998

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.782%

Totals Coverage Status
Change from base Build 6816647681: 0.0%
Covered Lines: 49032
Relevant Lines: 51191

💛 - Coveralls

Copy link
Member

@ameraner ameraner left a comment

Choose a reason for hiding this comment

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

LGTM, well spotted!

@pnuu pnuu merged commit 6fb1d1e into pytroll:main Nov 13, 2023
18 of 19 checks passed
@pnuu pnuu deleted the fci-duplicate-required-variables branch November 13, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code cleanup but otherwise no change in functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants