-
Notifications
You must be signed in to change notification settings - Fork 270
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
Storm-Relative Velocity #1654
Conversation
@EWolffWX Thanks for the PR! Next steps would be to include the function in the |
@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 |
@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: 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. |
Added storm_relative_velocity to __init__.py
@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! |
@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 |
@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. |
@EWolffWX - I would suggest using the file from AWS! You can use the s3-path |
@mgrover1 - Good to know, thanks Max! |
@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? |
@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. |
@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. |
Following up here @EWolffWX - let us know how we can help and if you have any other questions! |
@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 ? |
@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! |
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. |
@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 |
@mgrover1 This one should be good for a final review, the example could always be added in another PR. |
There was a problem hiding this 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 , @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! |
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.