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

Add scanline timestamps to seviri_l1b_hrit #752

Merged

Conversation

sfinkens
Copy link
Member

@sfinkens sfinkens commented May 6, 2019

  • Named it acq_time because it is a non-dimension coordinate, in contrast to time which is expected to be a dimension coordinate in the CF Writer (see also http://xarray.pydata.org/en/stable/data-structures.html?highlight=coordinates#coordinates).

  • Resampling drops the acquisition time which I think is correct

  • I changed the return value of satpy.readers.seviri_base.get_cds_time to datetime64, but it hasn't been used anywhere yet

  • Added a basic reader introduction and a recipe to enable .sel(time=...) via MultiIndex to the docs

  • Tests added

  • Tests passed

  • Passes git diff origin/master -- "*py" | flake8 --diff

  • Fully documented

@codecov
Copy link

codecov bot commented May 6, 2019

Codecov Report

Merging #752 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #752      +/-   ##
==========================================
+ Coverage   80.68%   80.75%   +0.06%     
==========================================
  Files         149      149              
  Lines       21650    21871     +221     
==========================================
+ Hits        17469    17662     +193     
- Misses       4181     4209      +28
Impacted Files Coverage Δ
satpy/readers/seviri_base.py 98.73% <100%> (+1.47%) ⬆️
satpy/tests/reader_tests/test_seviri_base.py 97.29% <100%> (+0.23%) ⬆️
satpy/readers/seviri_l1b_hrit.py 78.45% <100%> (+0.44%) ⬆️
satpy/tests/reader_tests/test_seviri_l1b_hrit.py 98.11% <100%> (+0.2%) ⬆️
satpy/readers/geocat.py 87.5% <0%> (-4.23%) ⬇️
satpy/node.py 91.42% <0%> (-2.86%) ⬇️
satpy/readers/goes_imager_nc.py 63.88% <0%> (-1.86%) ⬇️
satpy/readers/yaml_reader.py 93.03% <0%> (-0.33%) ⬇️
satpy/readers/grib.py 79.86% <0%> (-0.29%) ⬇️
satpy/tests/test_yaml_reader.py 98.51% <0%> (-0.26%) ⬇️
... and 9 more

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 d0ff651...539de3a. Read the comment docs.

@coveralls
Copy link

coveralls commented May 6, 2019

Coverage Status

Coverage increased (+0.07%) to 80.755% when pulling 539de3a on sfinkens:feature-scanline-timestamps-seviri-hrit into d0ff651 on pytroll:master.

@sfinkens sfinkens self-assigned this May 6, 2019
@sfinkens sfinkens added component:enhancements component:readers enhancement code enhancements, features, improvements and removed component:enhancements labels May 6, 2019
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.

Great PR as usual, thanks! It looks good, I just have a couple of questions, among which: can this be useful for other seviri formats ?

return datetime(1958, 1, 1) + timedelta(days=float(days),
milliseconds=float(msecs))
if isinstance(days, (int, float)):
days = np.array([days], dtype='int64')
Copy link
Member

Choose a reason for hiding this comment

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

What if days is a float ? Shouldn't something go over to msecs ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, that doesn't make sense. Both days and msecs are expected to be integer, so the check for float should be removed. It was meant to check whether the input is scalar. But maybe np.isscalar is a better option

days.astype('timedelta64[D]') + msecs.astype('timedelta64[ms]')

if len(time) == 1:
return time[0]
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed ? Don't we always have the time as an array with multiple values .?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. the scanline timestamps are always multiple values. The use case I had in mind are attributes which contain a single timestamp (day, msec) (not sure whether they exist). If scalars are provided to the function, it would return a scalar, not an array. But I agree that it might be confusing to have different return types.

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.

I think this is a good balance from what I can tell. Adds it as a coordinate, shows how to use it in a MultiIndex. I have the same questions as @mraspaud, but otherwise looks pretty good.

@sfinkens
Copy link
Member Author

sfinkens commented May 6, 2019

Oh, I completely forgot about the big picture 😅 In the native format each record has its own timestamp, but it should be relatively easy to provide it. Never used the netCDF format before. I will order one at EUM and see whether it includes scanline timestamps.

- Use np.isscalar to check for scalar input
- Replace 1958-01-01 00:00 with NaT in this method already
@sjoro
Copy link
Collaborator

sjoro commented May 7, 2019

nice work @sfinkens! i'm happy with this. i have the same remark as @mraspaud, this feature should be evaluated against other SEVIRI formats, too.

@sfinkens
Copy link
Member Author

sfinkens commented May 7, 2019

@sjoro Summarizing the discussion on slack:

@mraspaud
Copy link
Member

mraspaud commented May 7, 2019

The conclusion was we leave the scanline timestamps for the other SEVIRI readers to another PR

@mraspaud mraspaud merged commit e1e0d5a into pytroll:master May 7, 2019
@sfinkens sfinkens deleted the feature-scanline-timestamps-seviri-hrit branch May 7, 2019 09:04
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

5 participants