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

TCTracks: Option to read additional variables from IBTrACS (e.g. "nature"/stormtype) #728

Merged
merged 8 commits into from
May 31, 2023

Conversation

tovogt
Copy link
Collaborator

@tovogt tovogt commented May 23, 2023

Changes proposed in this PR:

  • Add a parameter additional_variables to the TCTracks.from_ibtracs_netcdf function that allows users to have additional IBTrACS data fields be included in the TCTracks object.

This PR fixes #674 and replaces #646

PR Author Checklist

PR Reviewer Checklist

@ChrisFairless
Copy link
Collaborator

This is great! Looks good, reads well, and everything works.

Only one remark: the additional variables include a few numeric fields (iso_time, wmo_wind, wmo_pres, dist2land, storm_speed, storm_dir). None of these are interpolated when interpolate_missing = True. Do we want to extend the code to interpolate missing values here or leave that to the user?

@tovogt
Copy link
Collaborator Author

tovogt commented May 25, 2023

Only one remark: the additional variables include a few numeric fields (iso_time, wmo_wind, wmo_pres, dist2land, storm_speed, storm_dir). None of these are interpolated when interpolate_missing = True. Do we want to extend the code to interpolate missing values here or leave that to the user?

These variables are not reported variables, but are provided by IBTrACS as a courtesy to users, e.g. dist2land, storm_speed, storm_dir. They never have missing values as far as I understand it. For wmo_wind and wmo_pres, I think that nobody should use these. If somebody is interested in those values, they should use the provider="official" feature.

The documentation of interpolate_missing includes a complete list of variables that are affected by this setting, and I think that's clear enough.

@ChrisFairless
Copy link
Collaborator

Thanks for clarifying! Then I think this is ready to merge.

Copy link
Collaborator

@bguillod bguillod left a comment

Choose a reason for hiding this comment

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

This looks good to me.
A big THANK YOU @tovogt for writing code and creating this PR, this is highly appreciated.

Copy link
Member

@peanutfun peanutfun left a comment

Choose a reason for hiding this comment

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

Great work, @tovogt! Since @chahank is a bit busy at the moment I took the liberty of reviewing your changes.

climada/hazard/tc_tracks.py Outdated Show resolved Hide resolved
climada/hazard/test/test_tc_tracks.py Outdated Show resolved Hide resolved
@tovogt tovogt removed the request for review from chahank May 31, 2023 12:49
@tovogt
Copy link
Collaborator Author

tovogt commented May 31, 2023

Thanks for your reviews, @ChrisFairless, @bguillod, @peanutfun !

From my side, this is ready to be merged.

@peanutfun
Copy link
Member

Great, thanks a lot. I'll merge then!

@peanutfun peanutfun merged commit c6851cd into develop May 31, 2023
@emanuel-schmid emanuel-schmid deleted the feature/additional-vars-from-ibtracs branch June 7, 2023 10:23
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