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

Change combine_metadata to average any 'time' fields #473

Merged
merged 7 commits into from
Oct 24, 2018

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Oct 23, 2018

Edit: As discussed below and on slack, this PR has been modified from its original purpose. It now does as the title states by modifying combine_metadata instead of modifying the ahi_hsd reader with a workaround (using scheduled time for start time). The original PR description is below:

Observation time, which was used previously, varies between bands and
can make compositing difficult (no resulting start_time). This uses the
scheduled time of the image which should be the same for all bands for
a particular region/sector.

I'm still going to add some documentation to the top of the reader module to mention this time difference.

  • Tests added
  • Tests passed
  • Passes git diff origin/master -- "*py" | flake8 --diff
  • Fully documented

Observation time, which was used previously, varies between bands and
can make compositing difficult (no resulting start_time). This uses the
scheduled time of the image which should be the same for all bands for
a particular region/sector.
@djhoese
Copy link
Member Author

djhoese commented Oct 23, 2018

I just realized that the observation_time metadata I'm adding to the DataArray isn't being kept in the final DataArray loaded in to the Scene. This is because there are multiple sectors (files) being loaded to make that single large data array and since the time doesn't match for all of them it isn't used. @mraspaud do you have any ideas for this?

This makes me think we need to modify the metadata merging functions so that they are semi-aware of 'time' keys (minimum of anything with 'time' in it except 'maximum' for 'time' that also has 'end' or 'stop' in it).

@coveralls
Copy link

coveralls commented Oct 23, 2018

Coverage Status

Coverage increased (+0.4%) to 73.967% when pulling d0a994e on djhoese:bugfix-ahi-hsd-time into 1942298 on pytroll:master.

@codecov
Copy link

codecov bot commented Oct 23, 2018

Codecov Report

Merging #473 into master will increase coverage by 0.38%.
The diff coverage is 93.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #473      +/-   ##
==========================================
+ Coverage   73.57%   73.96%   +0.38%     
==========================================
  Files         135      136       +1     
  Lines       17833    17954     +121     
==========================================
+ Hits        13121    13280     +159     
+ Misses       4712     4674      -38
Impacted Files Coverage Δ
satpy/readers/ahi_hsd.py 45.3% <100%> (+1.52%) ⬆️
satpy/tests/reader_tests/test_ahi_hsd.py 94.54% <100%> (+2.65%) ⬆️
satpy/dataset.py 89.56% <78.57%> (-1.62%) ⬇️
satpy/tests/test_dataset.py 97.72% <95%> (-2.28%) ⬇️
satpy/tests/reader_tests/test_hrit_msg.py 94.66% <0%> (ø)
satpy/tests/reader_tests/__init__.py 97.43% <0%> (+0.06%) ⬆️
satpy/readers/hrit_msg.py 44.63% <0%> (+23.16%) ⬆️

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 1942298...d0a994e. Read the comment docs.

satpy/dataset.py Outdated
if sys.version_info < (3, 3):
# timestamp added in python 3.3
import time
timestamp_func = lambda x: time.mktime(x.timetuple())
Copy link
Contributor

Choose a reason for hiding this comment

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

E731 do not assign a lambda expression, use a def

satpy/dataset.py Outdated
import time
timestamp_func = lambda x: time.mktime(x.timetuple())
else:
timestamp_func = lambda x: x.timestamp()
Copy link
Contributor

Choose a reason for hiding this comment

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

E731 do not assign a lambda expression, use a def

satpy/dataset.py Outdated
return datetime.fromtimestamp(sum(total) / len(total))


def combine_metadata(*metadata_objects, average_times=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

E999 SyntaxError: invalid syntax

@djhoese djhoese changed the title Fix start_time in ahi_hsd reader to be the scheduled time Change combine_metadata to average any 'time' fields Oct 23, 2018
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.

Looks good to me. Just a comment and a question: did you check that it doesn't break the time attributes in the aggregation of eg viirs granules ?

return datetime.fromtimestamp(sum(total) / len(total))


def combine_metadata(*metadata_objects, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

why not def combine_metadata(*metadata_objects, average_times=True):?

Copy link
Member Author

Choose a reason for hiding this comment

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

Read the comment below. That syntax isn't valid in python 2. I had it that way first and the tests failed. Unless you're saying I should drop python 2 support now... 😜

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy with that :)

@djhoese
Copy link
Member Author

djhoese commented Oct 24, 2018

Which time attributes specifically? The yaml_reader always overwrites start and end time with the first and last times of the file handlers.

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

4 participants