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

manually remove 'z' from datetime string #294

Merged
merged 7 commits into from
Mar 16, 2022
Merged

Conversation

JessicaS11
Copy link
Member

Per the segfault issue noted in #281, Xarray/pandas/numpy no longer automatically handle datetimes brought in with a timezone ('Z' at the end of the datetime string). This provides a fix by checking and manually removing the last character of the datetime string if it's a Z.

@JessicaS11 JessicaS11 requested a review from weiji14 March 16, 2022 16:39
@github-actions
Copy link

github-actions bot commented Mar 16, 2022

Binder 👈 Launch a binder notebook on this branch for commit 076d149

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit 35435d1

@ricardobarroslourenco
Copy link
Member

Does it need to merge this PR, with the other one ( #281 )?

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Really think we need a unit test for the Read module, do you have time to add one in?

icepyx/core/read.py Outdated Show resolved Hide resolved
@JessicaS11
Copy link
Member Author

Does it need to merge this PR, with the other one ( #281 )?

Yes - it turns out this was a standalone issue occurring for other datasets too (due to a deprecation in the most recent versions of numpy, v 1.22), so we gave it its own PR. Once we merge this into development, we can update #281 and fingers crossed that PR will be good to go too.

@JessicaS11
Copy link
Member Author

Really think we need a unit test for the Read module, do you have time to add one in?

Agreed. I won't have time until after the hackweek. : )

@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2022

Codecov Report

Merging #294 (a02bc41) into development (768c520) will decrease coverage by 0.08%.
The diff coverage is 0.00%.

@@               Coverage Diff               @@
##           development     #294      +/-   ##
===============================================
- Coverage        55.56%   55.47%   -0.09%     
===============================================
  Files               31       31              
  Lines             2014     2017       +3     
  Branches           412      413       +1     
===============================================
  Hits              1119     1119              
- Misses             825      828       +3     
  Partials            70       70              
Impacted Files Coverage Δ
icepyx/core/read.py 29.47% <0.00%> (-0.53%) ⬇️

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 768c520...a02bc41. Read the comment docs.

icepyx/core/read.py Outdated Show resolved Hide resolved
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Cool, thanks Jessica! I've tested locally on ATL06 and the data loads fine into xarray. Also tried on ATL08 and there's still the IndexError, but at least it doesn't crash. Not sure about the other ICESat-2 products 🙂

@JessicaS11 JessicaS11 merged commit aa61e3f into development Mar 16, 2022
@JessicaS11 JessicaS11 deleted the npdatetime branch March 16, 2022 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants