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

Feature/from netcdf fast #993

Merged
merged 27 commits into from
Feb 12, 2025
Merged

Feature/from netcdf fast #993

merged 27 commits into from
Feb 12, 2025

Conversation

NicolasColombi
Copy link
Collaborator

@NicolasColombi NicolasColombi commented Jan 16, 2025

Changes proposed in this PR:

This PR adds a method to import tropical cyclones tracks from an open source tc model called FAST.

  • from_FAST(): Import model output in netcdf format, restructure it, compute missing central pressure and radious of
  • test_from_FAST()

PR Author Checklist

PR Reviewer Checklist

@peanutfun
Copy link
Member

I haven't reviewed the changes, but I looked at this PR because it sounded like you implemented a faster way of reading NetCDF. It might be worthwhile to consider disobeying the naming scheme and uppercasing "FAST" in the method names (i.e., from_FAST) to clarify that it does not mean "quick" 😅

@NicolasColombi NicolasColombi marked this pull request as ready for review January 27, 2025 10:32
@NicolasColombi NicolasColombi marked this pull request as draft February 4, 2025 18:45
@NicolasColombi NicolasColombi marked this pull request as ready for review February 5, 2025 10:45
track.time.data, unit="s", origin=reference_time
).astype("datetime64[s]")
# Define variables
max_wind_kn = track.vmax_trks.data * 1.943844
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer defining a constant somewhere and avoid hardcoded values

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree, I was trying to avoid the pylint: too many locals 😬

@@ -162,6 +162,7 @@
"SI": 1005,
"WP": 1005,
"SP": 1004,
"AU": 1004,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be worth mentioning this addition to the changelog for track keeping!

Copy link
Collaborator

@spjuhel spjuhel 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 @NicolasColombi !

I suggested some very minor changes to make the code even better !

In addition, I am wondering if you could write a brief description in the tutorial on how this can be used (a few lines to exemplify how to run the FAST model, and use these functions on the folder). Not mandatory though.

@NicolasColombi
Copy link
Collaborator Author

👍 Great work @NicolasColombi !

I suggested some very minor changes to make the code even better !

In addition, I am wondering if you could write a brief description in the tutorial on how this can be used (a few lines to exemplify how to run the FAST model, and use these functions on the folder). Not mandatory though.

Thank you Sam! Good point, I will add it! I am wondering whether it should be a short part of the current tropical cyclone tutorial or if I should create a new one, any thoughts on that ? because with #1003 we will be adding an other feature that probably should appear in some tutorial as well and I am hesitant to make it even longer. So I guess, either a short section in the current tutorial or a more descriptive in a new one ?

@spjuhel
Copy link
Collaborator

spjuhel commented Feb 7, 2025

I think a brief one is quite enough, it is mostly to show off your work a bit more :)

climada/hazard/tc_tracks.py Outdated Show resolved Hide resolved
@NicolasColombi
Copy link
Collaborator Author

I think a brief one is quite enough, it is mostly to show off your work a bit more :)

Fine by me, I will add the documentation all at once in #1003 then.

@NicolasColombi
Copy link
Collaborator Author

@spjuhel as discussed, I'll merge.

@NicolasColombi NicolasColombi merged commit 3546a0b into develop Feb 12, 2025
15 of 19 checks passed
@NicolasColombi NicolasColombi deleted the feature/from_netcdf_fast branch February 12, 2025 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants