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 combination of raw metadata (again) #1670

Merged
merged 7 commits into from May 18, 2021

Conversation

sfinkens
Copy link
Member

In my recent attempt to fix the combination of raw metadata I updated satpy.dataset.metadata.combine_metadata to drop the raw_metadata attribute. This works fine with single-file datasets (e.g. SEVIRI Native), but for multi-file datasets (e.g. SEVIRI HRIT) the attribute is dropped even if only a single dataset has been loaded. This is of course not desired.

This PR enables combine_metadata to compare (nested) dictionaries, so that the removal of the raw_metadata attribute is not necessary anymore. In the end it wasn't as complicated as I thought.

  • Tests added

Instead of dropping raw metadata, enable combine_metadata to
recursively compare nested dictionaries.
@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #1670 (d15bb5b) into main (2a1110e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1670      +/-   ##
==========================================
+ Coverage   92.66%   92.67%   +0.01%     
==========================================
  Files         258      258              
  Lines       37988    38071      +83     
==========================================
+ Hits        35200    35283      +83     
  Misses       2788     2788              
Flag Coverage Δ
behaviourtests 4.84% <18.18%> (+0.01%) ⬆️
unittests 92.81% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
satpy/dataset/metadata.py 99.07% <100.00%> (+0.38%) ⬆️
satpy/tests/test_dataset.py 100.00% <100.00%> (ø)
satpy/dataset/dataid.py 94.05% <0.00%> (-0.23%) ⬇️
satpy/writers/cf_writer.py 95.17% <0.00%> (-0.02%) ⬇️
satpy/tests/test_multiscene.py 99.46% <0.00%> (+0.04%) ⬆️
satpy/writers/geotiff.py 90.90% <0.00%> (+0.16%) ⬆️
satpy/tests/writer_tests/test_geotiff.py 97.87% <0.00%> (+0.19%) ⬆️
satpy/multiscene.py 91.10% <0.00%> (+0.33%) ⬆️

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 2a1110e...d15bb5b. Read the comment docs.

@djhoese
Copy link
Member

djhoese commented May 12, 2021

While I'm OK with this, it is only partially because I don't use SEVIRI data on a regular basis. What kind of performance issues can we expect with something like this?

If they are significant, maybe dict comparison should be turned off by default and have a keyword argument to enable it? Then the SEVIRI file handlers could override the base combine_info method and either copy the first raw_metadata dictionary or enable the dict comparison with the keyword argument?

@sfinkens
Copy link
Member Author

For SEVIRI HRIT it increases the runtime of _combine_shared_info by 18%, but it's still only 2 milliseconds in total

satpy/dataset/metadata.py Outdated Show resolved Hide resolved
satpy/dataset/metadata.py Outdated Show resolved Hide resolved
sfinkens and others added 2 commits May 12, 2021 18:59
Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>
satpy/dataset/metadata.py Outdated Show resolved Hide resolved
@sfinkens sfinkens added this to In progress in PCW Spring 2021 via automation May 17, 2021
satpy/dataset/metadata.py Outdated Show resolved Hide resolved
@sfinkens sfinkens moved this from In progress to Review in progress in PCW Spring 2021 May 18, 2021
PCW Spring 2021 automation moved this from Review in progress to Reviewer approved May 18, 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.

Looks good from what I understand.

@djhoese djhoese merged commit 59903ae into pytroll:main May 18, 2021
PCW Spring 2021 automation moved this from Reviewer approved to Done May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants