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

Dolfyn updates + turbulence analysis #232

Merged
merged 23 commits into from
Aug 4, 2023

Conversation

jmcvey3
Copy link
Contributor

@jmcvey3 jmcvey3 commented Mar 28, 2023

A slew of bugfixes and inconsistency cleanup, as well as including ADCP turbulence functionality for #227

Changelog:

  • Bugfixes

    • Added check to ensure n_bin is shorter than the total data length when calling dolfyn.TimeBinner.reshape
    • Added checks to ensure n_fft and n_fft_coh can't be greater than n_bin
    • Fixed bug where dolfyn.adp.nan_beyond_surface overtrimmed TRDI instrument data
    • Fixed bug where dolfyn.ADVBinner.cross_spectral_density would fail if n_fft != n_fft_coh
    • Improved robustness of Nortek Signature reader for deployments not measuring water velocity
    • log file extension changed from ".log" to ".dolfyn.log"
  • API/Useability

    • Calculation of depth from pressure sensor updated to use linear approximation of the equation of state, rather than EOS-80
    • Added warnings for ADV motion and turbulence functions
    • Updated dataset.velds.U_dir shortcut to automatically convert "degrees CCW from X/East/streamwise" to "degrees CW from X/North/streamwise"
    • dolfyn.ADVBinner.cross_spectral_density now returns frequency coordinate coh_freq instead of freq
    • Added ADCP turbulence functions
    • Added function to calculate Doppler noise to ADV turbulence functions
    • Added "beam_angle" attribute to Nortek Signature datasets
    • Saved full Nortek Signature "config" dictionary as json string in attributes
    • Added warning if "rotate_vars" attribute not found
    • Standardized variable metadata based on CF conventions

@jmcvey3 jmcvey3 changed the title Dolfyn updates Dolfyn updates + turbulence analysis Mar 28, 2023
@ssolson
Copy link
Contributor

ssolson commented Apr 5, 2023

James pulling the latest develop branch should fix your test failures

ssolson and others added 3 commits April 14, 2023 11:16
* Update calls to grid formatting

* run on develop
…#225)

In an oversight in merging MHKiT-Software#193 the docstring was updated for the
type of return_year in return_year_value but the corresponding
assertion was not.

Tests for the types of the two input parameters have been added to
try to avoid a simular situation occuring the future.
@jmcvey3
Copy link
Contributor Author

jmcvey3 commented Apr 21, 2023

@browniea I've finished my updates for this PR, though I need to update documentation and add an example notebook. Was there something else you wanted to add to it?

@jmcvey3
Copy link
Contributor Author

jmcvey3 commented Jun 13, 2023

Currently working on adding turbulence functions to ADCP example notebook

@ssolson
Copy link
Contributor

ssolson commented Jun 29, 2023

@jmcvey3 in #236 you said:

@ssolson I just remembered there are some changes in the dolfyn PR (namely to the function/shortcut dolfyn.velocity.U_dir) that will affect this one. I'll try to wrap the dolfyn PR soon here so we can merge these back to back.

So we need to complete this PR first then #236? I am only confused because it seems #236 has passed all tests.

@ssolson ssolson changed the base branch from master to develop July 12, 2023 16:17
@jmcvey3
Copy link
Contributor Author

jmcvey3 commented Jul 18, 2023

@ssolson Just pushed some updates to the jupyter notebooks to incorporate the ADCP turbulence analysis.

@jmcvey3 in #236 you said:

@ssolson I just remembered there are some changes in the dolfyn PR (namely to the function/shortcut dolfyn.velocity.U_dir) that will affect this one. I'll try to wrap the dolfyn PR soon here so we can merge these back to back.

So we need to complete this PR first then #236? I am only confused because it seems #236 has passed all tests.

Just to have this on paper, it's a change relevant to the tidal performance jupyter notebook

@jmcvey3
Copy link
Contributor Author

jmcvey3 commented Jul 18, 2023

@ssolson Just pushed some updates to the jupyter notebooks to incorporate the ADCP turbulence analysis. This PR should be good to go for final review

Copy link
Contributor

@ssolson ssolson left a comment

Choose a reason for hiding this comment

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

James you did a lot of great work here. I have a number of minor suggestions primarily around docstrings. Outside of the new turbulence model this seems to be a great deal of clean up and improvements top the current dolfyn module.

I still need to review the notebooks next week.

Comment on lines 327 to 353
def calc_tilt(pitch, roll):
"""
Calculate "tilt", the vertical inclination, from pitch and roll.

Parameters
----------
roll : numpy.ndarray or xarray.DataArray
Instrument roll
pitch : numpy.ndarray or xarray.DataArray
Instrument pitch

Returns
-------
tilt : numpy.ndarray
Vertical inclination of the instrument
"""

if 'xarray' in type(pitch).__module__:
pitch = pitch.values
if 'xarray' in type(roll).__module__:
roll = roll.values

tilt = np.arctan(
np.sqrt(np.tan(np.deg2rad(roll)) ** 2 + np.tan(np.deg2rad(pitch)) ** 2)
)

return np.rad2deg(tilt)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably specify in the Parameters & Return that the function expects and returns degrees.

This may be well-known among practitioners. Is vertical inclination always measured from the z-direction?
So my understanding is that if pitch and roll are both 0 then tilt is zero and we have an instrument at perfect level. with 0 degrees being straight down in the water.

Then for non zero roll, pitch those degrees will vary between -90 and 90 which seems maybe slightly more problematic than having an always positive degree like 0 to 180. This is all mostly me just thinking out loud and making sure I understand. My guess is that it can be handled either way and the way you have done it here makes the most sense in this application

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, tilt for ADCPs is the vertical inclination and ranges from +/- 90 degrees for pitch and roll

mhkit/dolfyn/adv/motion.py Outdated Show resolved Hide resolved
mhkit/dolfyn/adv/motion.py Show resolved Hide resolved
mhkit/dolfyn/rotate/vector.py Show resolved Hide resolved
mhkit/dolfyn/rotate/vector.py Show resolved Hide resolved
mhkit/dolfyn/adp/turbulence.py Show resolved Hide resolved
mhkit/dolfyn/adp/turbulence.py Show resolved Hide resolved
mhkit/dolfyn/adp/turbulence.py Outdated Show resolved Hide resolved
mhkit/dolfyn/adp/turbulence.py Outdated Show resolved Hide resolved
mhkit/dolfyn/adp/turbulence.py Outdated Show resolved Hide resolved
@ssolson
Copy link
Contributor

ssolson commented Aug 3, 2023

@jmcvey3 please ping me when you are ready for me to take a look. On this PR I have jmcvey3#4 open agains your branch for my suggestions for the ipython notebook.

@jmcvey3
Copy link
Contributor Author

jmcvey3 commented Aug 3, 2023

@ssolson Ready to go! I cleaned up the rest of the docstrings (I think I got everything) and added more checks for function inputs.

@ssolson
Copy link
Contributor

ssolson commented Aug 4, 2023

James thank you making all those changes.

Currently the CI/CD has passed all but the hindcast which this PR does not modify. As such I am going to proceed with the merge.

image

@ssolson ssolson merged commit 7845dd2 into MHKiT-Software:develop Aug 4, 2023
16 of 18 checks passed
ssolson added a commit that referenced this pull request Aug 11, 2023
**MHKiT 0.7.0 Release Notes**

This release introduces exciting new features and improvements to the MHKiT package:

- **Mooring Module**: We are pleased to introduce the new mooring module. This addition primarily supports outputs from MoorDyn. Within this module, users can:
  - Import data
  - Calculate lay length
  - Visualize mooring line movements in 2D and 3D with graphical animations.
  
  Accompanying this module is an example notebook to guide users on its functionalities.

- **Dolfyn Module Revamp**: The Dolfyn module has been overhauled. Enhancements include:
  - Turbulence calculation capability
  - Performance measures for tidal power as outlined in IEC/TS 6200-200.

- **New Contributions**: A big shoutout to our community member, @mbruggs, for adding the ability to compute surface elevation using IFFT.

- **NDBC Buoy Metadata**: Users can now fetch NDBC buoy metadata directly through MHKiT.

- **Delft3D Module Update**: Stay up to date with support for the latest Delft3D NetCDF format.

**Additions**
- #235
- #232 
- #236 
- #250
- #239
- #248

**Bug Fixes**
- #226 
- #238

**Meta/Minor Changes**
- #220
- #243
- #225 
- #231
- #224

Thank you to all of the contributers who helpped with this release:
@mbruggs @Graham-EGI @castillocesar @jmcvey3 @hivanov-nrel  @browniea @cmichelenstrofer @akeeste  @maxwelllevin @rpauly18 @ssolson
@jmcvey3 jmcvey3 deleted the dolfyn_updates branch October 3, 2023 16:21
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