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.

Sign up for GitHub

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

Jump to bottom

CF Writer Improvements #976

Merged
merged 4 commits into from
Dec 10, 2019
Merged

CF Writer Improvements #976

merged 4 commits into from
Dec 10, 2019

Conversation

sfinkens
Copy link
Member

@sfinkens sfinkens commented Nov 25, 2019 •
edited
Loading

  • Fix crash with unlimited dimensions
  • Align units of time coordinates and bounds. Currently we are saving absolute times but relative time bounds (seconds since start time)
  • Sort datasets by name
  • Tests added and test suite added to parent suite
  • Tests passed
  • Passes flake8 satpy

@sfinkens sfinkens added bug enhancement code enhancements, features, improvements PCW Pytroll Contributors' Week component:writers labels Nov 25, 2019
@sfinkens sfinkens self-assigned this Nov 25, 2019
sfinkens
sfinkens commented Nov 25, 2019
View reviewed changes
satpy/writers/cf_writer.py
time_bnds = [(np.datetime64(start_time) - dtnp64),
(np.datetime64(end_time) - dtnp64)]
data = xr.DataArray(np.array(time_bnds)[None, :] / np.timedelta64(1, 's'),
data = xr.DataArray([[np.datetime64(start_time), np.datetime64(end_time)]],
Copy link
Member Author

Choose a reason for hiding this comment

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

I"m not entirely sure, why the current method uses the timestamp. But I guess it"s just for encoding reasons (bounds relative to timestamp). By changing the bounds" datatype to datetime64 xarray will handle the encoding.

@sfinkens sfinkens removed the enhancement code enhancements, features, improvements label Nov 25, 2019
@sfinkens sfinkens force-pushed the fix-cf-writer-unlimited-dims branch from 20adf55 to b02bbdb Compare November 25, 2019 16:57
@coveralls
Copy link

coveralls commented Nov 25, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 86.872% when pulling 12171a6 on sfinkens:fix-cf-writer-unlimited-dims into 1fbfda5 on pytroll:master.

@codecov
Copy link

codecov bot commented Nov 25, 2019 •
edited
Loading

Codecov Report

Merging #976 into master will increase coverage by 0.01%.
The diff coverage is 96.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #976      +/-   ##
==========================================
+ Coverage   86.86%   86.87%   +0.01%     
==========================================
  Files         181      181              
  Lines       27461    27491      +30     
==========================================
+ Hits        23853    23883      +30     
  Misses       3608     3608
Impacted Files Coverage Δ
satpy/tests/writer_tests/test_cf.py 98.46% <100%> (+0.08%) ⬆️
satpy/writers/cf_writer.py 91.08% <90.47%> (-0.19%) ⬇️
satpy/scene.py 90.53% <0%> (+0.17%) ⬆️

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 1fbfda5...12171a6. Read the comment docs.

@sfinkens sfinkens mentioned this pull request Nov 25, 2019
Merged
2 tasks
@sfinkens sfinkens added component:enhancements enhancement code enhancements, features, improvements and removed component:enhancements labels Nov 25, 2019
mraspaud
mraspaud approved these changes Dec 10, 2019
View reviewed changes
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, thanks for simplifying the time bounds handling!

@mraspaud mraspaud merged commit 74a909a into pytroll:master Dec 10, 2019
@sfinkens sfinkens deleted the fix-cf-writer-unlimited-dims branch October 18, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mraspaud mraspaud mraspaud approved these changes

@djhoese djhoese Awaiting requested review from djhoese

Assignees

@sfinkens sfinkens

Labels
bug component:writers enhancement code enhancements, features, improvements PCW Pytroll Contributors' Week
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sfinkens @coveralls @mraspaud