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

Adapt tc_forecast to climada_python improved hdf5 IO #83

Merged
merged 2 commits into from
Jul 3, 2023

Conversation

emanuel-schmid
Copy link
Contributor

@emanuel-schmid emanuel-schmid commented Jun 29, 2023

Changes proposed in this PR:

  • cast "time" values to float when reading bufr file in read_one_bufr_tc
    I suspect that with climada_python PR #735 time values are of dtype float when read from hdf5 (which makes sense too). But so far time has been an int.

This PR fixes failing of climada_petals.hazard.test.test_tc_tracks_forecast.TestECMWF.test_hdf5_io due to type differences in tc_track from bufr file and tc_read from hdf5

PR Author Checklist

PR Reviewer Checklist

@emanuel-schmid emanuel-schmid changed the title Adapt tc_forecast to climada_python/[#735] Adapt tc_forecast to climada_python improved hdf5 IO Jun 29, 2023
@tovogt
Copy link
Collaborator

tovogt commented Jun 29, 2023

Actually, you mean "time_step" instead of "time", right? In fact, "time_step" needs to be a float, otherwise interpolation of track data to sub-hourly resolution will make problems. This has not been introduced by CLIMADA-project/climada_python#735, but it was a problem before already. A similar issue is fixed in CLIMADA-project/climada_python#741, by the way, where I fix the problem that from_simulations_emanuel stores the "time_step" variable as integer type.

@tovogt
Copy link
Collaborator

tovogt commented Jun 29, 2023

As for the proposed changes: I think it's fine, but I would suggest to add a test for the dtype of "time_step" in https://github.com/CLIMADA-project/climada_petals/blob/cf6c0772c24fff6bcb92d2238cfb28427c5389cf/climada_petals/hazard/test/test_tc_tracks_forecast.py#L61C56-L62C59

@emanuel-schmid
Copy link
Contributor Author

Many thanks! I'll add a a test.

@emanuel-schmid
Copy link
Contributor Author

test added

Copy link
Collaborator

@tovogt tovogt left a comment

Choose a reason for hiding this comment

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

Great, thanks! Ready to be merged from my side.

@emanuel-schmid emanuel-schmid merged commit f3b5824 into develop Jul 3, 2023
@emanuel-schmid emanuel-schmid deleted the feature/fix_tcforecast_time_dtype branch August 9, 2023 10:47
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.

2 participants