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 station time #520

Merged
merged 7 commits into from
Apr 12, 2023
Merged

Change station time #520

merged 7 commits into from
Apr 12, 2023

Conversation

fschlueter
Copy link
Collaborator

Do you like this change?

I think it makes the setter more readable and also allows to set unix time stamps. However, now the format with which it is stored is not fixed anymore. I do not see why this is a problem but maybe someone can correct me.

…at is not controlled/converted during setting but only during getting. This makes the code more readable but the state interally is undefined (but its only the format)
Allow to set station._station_time = None
Copy link
Collaborator

@sjoerd-bouma sjoerd-bouma left a comment

Choose a reason for hiding this comment

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

Looks like an improvement to me. Once some minor comments have been addressed and the tests have completed I think I can approve this. Thanks Felix!

NuRadioReco/framework/base_station.py Outdated Show resolved Hide resolved
NuRadioReco/framework/base_station.py Outdated Show resolved Hide resolved
NuRadioReco/framework/base_station.py Outdated Show resolved Hide resolved
fschlueter and others added 3 commits March 30, 2023 19:17
Fix Sjoerds comments
Add format to interface
@fschlueter
Copy link
Collaborator Author

fschlueter commented Apr 3, 2023

I realised that the station_time is also stored in the event_header of a nur file but as astropy object (which is a bad idea and was already changed for the actual station serialisation). I fixed that but for reading allow backwards compatibility. I removed this weird set date_hms snippet because I think it is now obsolet.

BTW, what is stored in the parameter station_time (21)? It is not actually used to store the station time, right? But then it is used for the event header as dict key?

@fschlueter
Copy link
Collaborator Author

Do we want to bump the nur file version for these changes?

@fschlueter
Copy link
Collaborator Author

@sjoerd-bouma

Copy link
Collaborator

@sjoerd-bouma sjoerd-bouma left a comment

Choose a reason for hiding this comment

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

I didn't realise that we already did things this way in base_station.py, so that this is basically just a fix for the event_header. I think this can be merged without further ado.

One thing I'm not entirely sure about is if we currently actually have ~ns level precision on the station time. E.g. if I set an astropy.time.Time object with a np.float128 and then call .value, it seems to return a np.float64 object with the associated loss in precision by default. But I think that is a separate issue.

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.

None yet

2 participants