-
Notifications
You must be signed in to change notification settings - Fork 48
Delft3D io functions #132
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
Delft3D io functions #132
Conversation
* changing path to data following organizational update * Reorganized headers Co-authored-by: rpauly18 <[email protected]> Co-authored-by: rpauly18 <[email protected]>
|
@browniea could you update your PR description at the top? I think you just need to delete the TODO and maybe very briefly discuss what the example notebook covers. |
rpauly18
left a comment
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.
Overall great PR. I have added a few comments about things to be addressed and questions for consideration.
examples/Delft3D_example.ipynb
Outdated
| "source": [ | ||
| "variable= 'ucx' \n", | ||
| "var_data_df= d3d.get_all_data_points(d3d_data, variable, time_index=1)\n", | ||
| "var_data_df\n", |
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.
@browniea were you trying to print out the var_data_df? If so, you will need either an explicit print statement or move the plot limits to a new cell.
| @@ -0,0 +1,964 @@ | |||
| { | |||
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.
Overall in the example notebook, I recommend more in line comments that explain what you are doing. The test descriptions are great, but they can get little overwhelming for novice programmers. Comments next to the code can help break it up more and explain things a bit more step by step.
mhkit/river/io/d3d.py
Outdated
|
|
||
| def create_points(x, y, z): | ||
| ''' | ||
| Turns x, y, and z into a DataFrame of points to interpolate over. |
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.
This could be a bit more descriptive to make the use case more clear. Might just be as simple as pointing users to the example notebook for example of use.
| return all_data | ||
|
|
||
|
|
||
| def unorm(x, y ,z): |
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.
@ssolson do you think this function would be good to have in the utils module? Its very general and could be useful elsewhere...
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.
Agree. @browniea please move this to the utils module.
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.
Added to utils @ssolson did you want me to remove it from this module?
| return unorm | ||
|
|
||
|
|
||
| def turbulent_intensity(data, points='cells', time_index= -1, |
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.
@ssolson similarly I wonder if we would want to move this into river.resource. Eventually this type of functionality will exist for ADCP data as well. Those functions will likely end up being a bit different, but technically one could use another data source then d3d to calculate turbulence intensity. What are your thoughts?
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.
@rpauly18 this function specifically takes the D3D NetCDF data structure and returns Turbulent Intensity so its a purpose-built specialized function. A general TI function in the river module would be useful but would require additional development beyond the scope of this PR IMO.
This commit would allow for importation of d3d type files.
get_layer _data: Get variable data from netcdf4 object at specified layer and timestep.creat_points: Turns x, y, and z into a DataFrame of points to interpolate over,Must be provided at least two points and at most one array
grid_data: convert multiple variables to the same gridget_all_data_points: Get data points from all layers in netcdf4 file generated from Delft3Dunorm: Calculates the root mean squared value given three arraysturbulent_intensity: Calculated the turbulent intensity for a given data set for the specified pointsThe Delft3D_example notebook goes several plot options for a turbine in a flume. These options include a centerline plot, a plane contour plot and a cross section contour plot.