MHKiT-Software / MHKiT-Python

MHKiT-Python provides the marine renewable energy (MRE) community tools for data processing, visualization, quality control, resource assessment, and device performance.
https://mhkit-software.github.io/MHKiT/
BSD 3-Clause "New" or "Revised" License
47 stars 45 forks source link

Improve Reynolds stress ADCP estimation notebook discussion #316

Closed jmcvey3 closed 1 month ago

jmcvey3 commented 2 months ago

Editing language based on more insight I've gained from reading literature on ADCP Reynolds stress and TKE measurements. The main edits here are to the adcp_example notebook and the removal of the "total_tke" function. Based on more recent lit, ADCPs struggle to measure the u'u' and v'v' normal stresses close to the seafloor and in tidal channels or other regions (turbine wakes) where vertical flow is not insignificant.

I also updated the ADV notebook to cover all the functions

ssolson commented 2 months ago

On the removal of total_turbulent_kinetic_energy could you tell me more?

Is this so wrong as to not be useful 100% of the time? You specifically call out ADCP, do ADVs suffer from the same issue? If not do they have a they own function for calculating total_turbulent_kinetic_energy? Is there any reason we would want to allow people access to this function? E.g. we could make it a hidden method so it is not recommended but exists for the determined individual.

jmcvey3 commented 1 month ago

Yes - the data you get from ADCPs requires two critical assumptions. 1. There is next to no vertical motion from the time a parcel of water passes through one beam before it passes through another ("assumption of homogeneity" in the lit). 2. One can't make inferences of turbulent scales smaller than the distance between each of the beams.

ADCPs seem to struggle to measure the u'u' and v'v' TKE components because assumption 1 isn't always true. The second assumption renders the total TKE less useful because the information contained in smaller turbulent scales, which might be critical, is lost.

I wrote the function maybe 8 months ago because TKE is one of the parameters our modeling team uses for validating tidal models. I ultimately want to remove it because my experiences with it so far have been quite off from ADV measurements and model output. The debate is still ongoing on how good ADCP estimations are for TKE/Reynolds stresses, but it likely depends on the measurement site.

ADVs don't have either of these problems because they're making a point measurement, and are still considered the de facto "ground truth" when analyzing turbulence data. (Shear probes also fall under this category.) The calc_tke function under the ADVBinner outputs each of the TKE components; it's trivial to add the components together to get total TKE.

ssolson commented 1 month ago

Hey @jmcvey3 apologies for messing this PR up. This should be easier going forward but there were some inconsistencies in how PR merged strategies were applied in the previous release by the final PR merge author which made this messier. I perhaps should have just accepted the mess but instead decided to squash into master. My preferred strategy going forward is to squash PRs to develop and merge by ort into master.

To ensure master and develop are even I did a force rebase on origin develop.

That said can you either rebase or resubmit this PR? The official approach is to:

git checkout develop
git fetch origin
git reset --hard origin/develop

git checkout feature-branch
git rebase develop
git push user-fork feature-branch --force

That said rebasing may be extremely painful and it might be easier to just start from the new develop and remake the changes. Apologies for the trouble on this.

jmcvey3 commented 1 month ago

Hey @jmcvey3 apologies for messing this PR up. This should be easier going forward but there were some inconsistencies in how PR merged strategies were applied in the previous release by the final PR merge author which made this messier. I perhaps should have just accepted the mess but instead decided to squash into master. My preferred strategy going forward is to squash PRs to develop and merge by ort into master.

To ensure master and develop are even I did a force rebase on origin develop.

That said can you either rebase or resubmit this PR? The official approach is to:

git checkout develop
git fetch origin
git reset --hard origin/develop

git checkout feature-branch
git rebase develop
git push user-fork feature-branch --force

That said rebasing may be extremely painful and it might be easier to just start from the new develop and remake the changes. Apologies for the trouble on this.

No worries, been there done that. I created a new PR