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

Storm-Relative Velocity #1654

Merged
merged 11 commits into from
Nov 26, 2024
Merged

Storm-Relative Velocity #1654

merged 11 commits into from
Nov 26, 2024

Conversation

EWolffWX
Copy link
Contributor

Adds script to compute storm relative velocity for a radar object using the existing radial velocity field. Script allows the user to specify the storm motion using either speed and direction or u and v components.

@EWolffWX EWolffWX requested a review from zssherman as a code owner September 24, 2024 22:43
@EWolffWX
Copy link
Contributor Author

@mgrover1 and @scollis, let me know what other steps I should take to get this incorporated! And thank you in advance for all your help!

@zssherman
Copy link
Collaborator

@EWolffWX Thanks for the PR! Next steps would be to include the function in the __init__.py in the retrieval module. We will want to add unit tests as well.

@EWolffWX
Copy link
Contributor Author

@zssherman Apologies in advance for all my basic questions, this is my first time contributing to a package. I've gone ahead and added to the __init__.py file (I'm guessing this will require a new pull request). I'm not familiar with unit tests however. Would you be able to walk me through that?

@zssherman
Copy link
Collaborator

@EWolffWX No worries at all! And the neat thing with the PR system is, if you do the init change to your branch that you are using for the PR (looks like your using your main branch on your fork), the PR will automatically update. Yeah the unit tests we can assist with., one example is:
https://github.com/ARM-DOE/pyart/blob/14f625a10633e92c12818c41ea9269322b25e1ee/tests/retrieve/test_advection.py#L46C1-L58C44

Essentially we are testing what is expected as the result, if the result changes, we know we broke something.

But we can assist more with that.

@EWolffWX
Copy link
Contributor Author

@zssherman Ah okay, this is starting to make more sense! I've updated the init file now. And okay, the premise behind the unit tests makes sense too!

@zssherman
Copy link
Collaborator

@EWolffWX I think I have time this week to work on this, did you perchance have a specific file you were testing the code on? I figured we can use that for unit testing

@EWolffWX
Copy link
Contributor Author

EWolffWX commented Oct 8, 2024

@zssherman Okay, great! I was mainly testing by reading in NEXRAD files from AWS, but I also have a cfradial file with a single sweep from the COW radar that I used to fix some indexing issues. Would either of those work better than the other for this? Alternatively, I can download a NEXRAD file from AWS.

@mgrover1
Copy link
Collaborator

mgrover1 commented Oct 8, 2024

@EWolffWX - I would suggest using the file from AWS! You can use the s3-path

@EWolffWX
Copy link
Contributor Author

EWolffWX commented Oct 8, 2024

@mgrover1 - Good to know, thanks Max!

@zssherman
Copy link
Collaborator

zssherman commented Oct 16, 2024

@EWolffWX Did you by chance have an example or specific date of the file you were testing on? Was going to see if I can write up some unit tests. Or did you also have an example you would like to add to the examples directory for Py-ART?

@EWolffWX
Copy link
Contributor Author

EWolffWX commented Oct 16, 2024

@zssherman - Yes, sorry for not passing this along! Here's an s3 path for a good example ("s3://noaa-nexrad-level2/2022/03/31/KDGX/KDGX20220331_012808_V06"). And I would be happy to throw together a short notebook example for the Py-ART example directory as well! Just let me know how I should pass that along to you all.

@zssherman
Copy link
Collaborator

@EWolffWX No worries and thank you! I can see if I can get some tests going. For the example, if your willing to put together a python file called, plot_storm_relative_velocity.py in the retrieve folder under examples, that would be great! The 'plot' in the title is so sphinx knows to generate that file for the documentation.

@mgrover1
Copy link
Collaborator

Following up here @EWolffWX - let us know how we can help and if you have any other questions!

@EWolffWX
Copy link
Contributor Author

@mgrover1 - Thanks! I've fallen a little behind with conference travel this last week, but I'll get that example made soon. Is there anything else I should be working on in regards to testing @zssherman ?

@zssherman
Copy link
Collaborator

@EWolffWX Apologies for the late response. The testing I can work with that file you provided. I might just need help confirming the return values look correct. Besides that, just an example and PEP8 fixes (I can fix the PEP8) and this should be good to go!

@zssherman
Copy link
Collaborator

Also do you have an example direction and U and V you were testing @EWolffWX for that file? I was going to use those parameters for the unit tests.

@EWolffWX
Copy link
Contributor Author

@zssherman - No worries, I can definitely help out with confirming values. And I have the example on my to-do list for this week, so hoping to get that done soon. For that case, I think I was using NE (45 degrees) at 20 m/s. So I think that would be u= 14.14 and v= 14.14 if my math is right

@zssherman
Copy link
Collaborator

@EWolffWX @mgrover1 I went ahead and added some unit tests. Let me know how it looks. I also added a check for speed as the Error wasn't being caught for if speed was missing.

@zssherman
Copy link
Collaborator

@mgrover1 This one should be good for a final review, the example could always be added in another PR.

Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Nice work @EWolffWX !! This looks great on my side.

@zssherman zssherman merged commit 1178574 into ARM-DOE:main Nov 26, 2024
15 checks passed
@EWolffWX
Copy link
Contributor Author

EWolffWX commented Dec 1, 2024

@zssherman , @mgrover1 - Hooray! Thank you both for your help!! And I'll definitely get that example put together in the coming weeks, I unexpectedly got swamped with paper revisions these last few weeks. Thanks again!

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