Coastal-Imaging-Research-Network / cBathy-Toolbox

Routines needed to run cBathy + demos
https://github.com/Coastal-Imaging-Research-Network/cBathy-toolbox/wiki/cBathy-User-Manual
GNU General Public License v3.0
25 stars 23 forks source link

Wrap alpha estimates and out-of-bounds quality control to the range [-pi pi) #35

Closed dahonegger closed 6 years ago

dahonegger commented 6 years ago

This fix wraps the difference between estimated and seed alpha to the range [-pi pi) before applying out-of-bounds quality control. The fix is needed because alpha estimates are initialized (in kAlphaPhiInit.m) to the range [-2pi 0] and estimated (via the least squares fit in csmInvertKAlpha.m) in an unwrapped range, but then good alpha estimates are assumed to all lie within pi/2 radians of the seed. This fix also wraps the alpha estimates themselves so that the estimates lie in a standard, wrapped range.

KateBrodie commented 6 years ago

@dahonegger @RobHolman @mpalmsten

I have completed an initial review of the pull request. I ran cBathy v1.2 as well as the version of the code on this pull request, herein referred to as PR35, on the 15 test cases from Holman et al. 2013. Please note, I compared fCombined output NOT the Kalman-filtered output as I was most interested in any differences in the estimates that would go in to the Kalman filter. SO, it's important to remember that the RMSE & biases reported here for the 15 test cases are larger than the RMSEs and biases reported in Holman et al. 2013.

Overall, this version of cBathy was quite comparable to v1.2 for the ideal cases examined here (see Figure below for comparison between RMSE, bias, and number of non-NaNs in the fCombined output for v1.2 (blue) and PR35 (red).

cbathy_statisticalcomparison_v1-2_pr35

In most cases, the results are pretty similar, as illustrated below graphically for Test Case 1:

examplecomparison1

However, in some cases, PR35 has a higher overall RMSE (e.g. Test Case 14). Examination of the output reveals that this is due to erroneous solve points on land near 700-800 in the alongshore in PR35, that are now considered good data by the algorithm (see Figure below) and therefore included in my calculation of RMSE (for now).

examplecomparison14

Open to input on further tests, however, overall this Pull Request fixes a bug and does not appear to change the results for the worse too drastically.

RobHolman commented 6 years ago

Just to be clear, this is just changing the small error in the angle seed?

Nice work, btw.

Rob Holman SECNAV/CNO Chair in Oceanography

104 Ocean Admin Bldg. CEOAS-OSU Corvallis, Oregon, USA 97331-5503 ph: 1-541-737-2914 holman@coas.oregonstate.edu http://cil-www.coas.oregonstate.edu

On May 29, 2018, at 10:47 AM, Katherine Brodie notifications@github.com wrote:

I have completed an initial review of the pull request. I ran cBathy v1.2 as well as the version of the code on this pull request, herein referred to as PR35, on the 15 test cases from Holman et al. 2013. Please note, I compared fCombined output NOT the Kalman-filtered output as I was most interested in any differences in the estimates that would go in to the Kalman filter. SO, it's important to remember that the RMSE & biases reported here for the 15 test cases are larger than the RMSEs and biases reported in Holman et al. 2013.

KateBrodie commented 6 years ago

@RobHolman yes, correct - this is just the fix submitted by @dahonegger for the wave angles.

I did not directly compare the wave angle output - I can do that if we think that makes sense as well.

dahonegger commented 6 years ago

Thank you for taking the time to do this!

A couple questions:

~David

bergsmaE commented 6 years ago

Hi all!

Nice work!

I am running testcases, all included: small/large tidal range and moderate wave conditions.

I have been running all versions since 2013 that I know, from Holman 13, Bergsma 14/16 with inter-camera boundary solution and a solution to cope with larger tidal ranges. Last test that I did was with the inter-camera boundary solution of Rob and the deep water solution. I will be in the US next week and let’s discuss in more detail then. I strongly believe we need to standardise this process with a set of known cases.

All the best,

Erwin Bergsma

+336 49346386

Op 29 mei 2018 om 20:24 heeft dahonegger notifications@github.com<mailto:notifications@github.com> het volgende geschreven:

Thank you for taking the time to do this!

A couple questions:

~David

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/Coastal-Imaging-Research-Network/cBathy-Toolbox/pull/35#issuecomment-392884602, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AYV9x97-QKTc7uIebptH3SJn_DJLCOUwks5t3ZJigaJpZM4Td9WT.

KateBrodie commented 6 years ago

@dahonegger good points

(1) for clarification, these are just the fCombined RMSE and bias for noon on the date provided in the Holman et al 2013 paper (so not a good comparison back to that paper which used the kalman filtered result after 4 days of running at noon on each day).

(2) i can write a script that pulls the wave data from our THREDDS and adds a comparison plot to these figures. Good point about this only affecting one quadrant.

Thanks for the feedback!

KateBrodie commented 6 years ago

@bergsmaE I totally agree! I suggested to @mpalmsten that this be one of our discussion topics next week - deciding on a standard set of test cases and providing testing scripts to show the differences in any code updates to make sure that we are in fact improving the algorithm with each new update. Looking forward to next week!