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

Deal with Nan timestamps #388

Merged
merged 4 commits into from
May 3, 2020
Merged

Deal with Nan timestamps #388

merged 4 commits into from
May 3, 2020

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Apr 30, 2020

As discussed in #386, adding support for nan values in unix_tai_to_utc

I also renamed the function unix_tai_to_time as it does not actually convert to utc.
It correctly creates an astropy.Time object, that is still in tai but converts to unix utc timestamp when accessing .unix.

For convenience and clarity, I would suggest to compare times to INVALID_TIME as now defined in the module.

@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #388 into master will increase coverage by 0.14%.
The diff coverage is 86.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
+ Coverage   45.47%   45.62%   +0.14%     
==========================================
  Files          64       64              
  Lines        4723     4736      +13     
==========================================
+ Hits         2148     2161      +13     
  Misses       2575     2575              
Impacted Files Coverage Δ
lstchain/reco/r0_to_dl1.py 64.18% <25.00%> (ø)
lstchain/reco/tests/test_utils.py 100.00% <100.00%> (ø)
lstchain/reco/utils.py 67.96% <100.00%> (+2.13%) ⬆️

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 b00cb78...e97b71b. Read the comment docs.

Copy link
Member

@morcuended morcuended left a comment

Choose a reason for hiding this comment

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

Seems fine to me. Do we still need to define missing time values as 0 or does this take into account the possibility of encounter nan values?

@maxnoe
Copy link
Member Author

maxnoe commented May 1, 2020

@morcuended it deals with nan like it would be zero.

So your choice now, what you want timestamp to be if it is missing, the conversion functin now supports both

@morcuended
Copy link
Member

@morcuended it deals with nan like it would be zero.

So your choice now, what you want timestamp to be if it is missing, the conversion functin now supports both

not strong preference, I would leave it as it is now with nan provided it works

@rlopezcoto
Copy link
Contributor

what about the interpolation, is it dealing with it properly?

@maxnoe
Copy link
Member Author

maxnoe commented May 1, 2020

Can you point me to the interpolation code?

@morcuended
Copy link
Member

Can you point me to the interpolation code?

functions linear_imputer and impute_pointing in lstchain/reco/utils.py which interpolate missing alt/az coordinates directly. Thus missing time values should not be a problem.

@morcuended
Copy link
Member

May you update the branch with the last merge fixing the muon output filename so I can test it?

@maxnoe
Copy link
Member Author

maxnoe commented May 1, 2020

Thus missing time values should not be a problem.

These are not missing anymore but 1970-01-01, so maybe you need to explicitly filter with time != INVALID_TIME

@rlopezcoto
Copy link
Contributor

@morcuended did you test this? can we move ahead with the merge?

@morcuended
Copy link
Member

@morcuended did you test this? can we move ahead with the merge?

Yes, I tested that the script runs well without problems and files are produced normally. However, I did not have the time to look into the time values themselves.

@rlopezcoto rlopezcoto merged commit e96c0cf into master May 3, 2020
@rlopezcoto rlopezcoto changed the title Nan timestamp Deal with Nan timestamps May 3, 2020
@maxnoe maxnoe deleted the nan_timestamp branch May 4, 2020 07:51
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.

3 participants