Mpalmsten nlinfit update

[mpalmsten]

I removed dependency on the Matlab Statistics and Machine Learning Toolbox by removing all calls to nlinfit.m, nlparci.m, and tinv.m and replacing with LMFnlsq.m and tstat3.m downloaded from the Matlab File Exchange. cBathy depth results have only very minimal differences with results from the development branch due to a slightly different implementation of the Levenberg-Marquardt curve fitting algorithm.

[KateBrodie]

@mpalmsten I am reviewing this now - Running on the same data as the last PR.

A couple of initial things I noticed (because I was getting all NaNs at first).

(1) you need to also update argus02b.m to add the .nlinfit parameter (argus02a has it)

(2) if the params.nlinfit parameter doesn't exist in the params file, cBathy appears to run (no errors) but returns all nans for fDependent and fCombined. This is because csmInvertAlpha is the first place to utilize that param, but it ocurrs inside a try/catch loop (line 139 to 145). the try/catch loop catches the error that parms.nlinfit doesn't exist, but all the catch component does is fill kAlphaPhi with nans (line 182), which then ends up causing fDependent to be all nans, and so on. I would suggest we add something that indicates why everything is nans in the catch loop - either print an error message to the screen or return the error message as an output within the bathy structure. @jstanleyx any ideas?

(3) the description of the params.nlinfit settings in argus02a references Richie's lsr code, which I don't think you ended up using, right?

If (2) above is too much to fix before the bootcamp we can add it to the to-do list for future development. I will let you know how the statistics look vs. nlinfit when it finishes processing in a bit.

[KateBrodie]

@mpalmsten @RobHolman @jstanleyx

I ran this pull request (herein PR38) with the new nonlinear solver on the 15 files from Holman et al 2013 and compared fCombined RMSE and bias relative to the survey data to the RMSE and bias of the current state of the development branch. Overall they are fairly comparable (see figure below). The PR38 with params.nlinfit = 0 runs about twice as slow on my computer than Development (5 min vs. 2.5 minutes).

The biggest differences are for test case 3 and 14 (fCombined plotted below for each). Test 14 is really only different in the 650 < y < 1000 and 100 < x < 150 region (similar to where PR35 was different from v1.2), and test case 3 is mostly different in the 100< x < 150 region as well, with a few differences elsewhere across the domain.

Overall, I am ok with approving once 1 and 2 in my comment above are addressed! Open to feedback though.

[SRHarrison]

that's cool. very promising.

[RobHolman]

I’m short of catching this from the airport so I’m not sure if I am getting it all. But my preference would be not to mess around in the catch loop, but instead do error checking with the params input. Actually John had some thoughts on this so we should discuss in St. Pete.

YHS

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 Jun 2, 2018, at 8:25 AM, Katherine Brodie notifications@github.com wrote:

@mpalmsten I am reviewing this now - Running on the same data as the last PR.

A couple of initial things I noticed (because I was getting all NaNs at first).

(1) you need to also update argus02b.m to add the .nlinfit parameter (argus02a has it)

(2) if the params.nlinfit parameter doesn't exist in the params file, cBathy appears to run (no errors) but returns all nans for fDependent and fCombined. This is because csmInvertAlpha is the first place to utilize that param, but it ocurrs inside a try/catch loop (line 139 to 145). the try/catch loop catches the error that parms.nlinfit doesn't exist, but all the catch component does is fill kAlphaPhi with nans (line 182), which then ends up causing fDependent to be all nans, and so on. I would suggest we add something that indicates why everything is nans in the catch loop - either print an error message to the screen or return the error message as an output within the bathy structure. @jstanleyx any ideas?

(3) the description of the params.nlinfit settings in argus02a references Richie's lsr code, which I don't think you ended up using, right?

If #2 above is too much to fix before the bootcamp we can add it to the to-do list for future development. I will let you know how the statistics look vs. nlinfit when it finishes processing in a bit.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

[mpalmsten]

Thanks Kate for running the standard test. I appreciate you putting that together, and I'm looking forward to learning more about it this week!

I addressed issues 1 and 3 from your review. I agree with your comment in 2 that we should run a check to make sure we have all inputs when we read in the params file (e.g., before we call analyzeBathyCollect.m). I think we should hold off until after the boot camp.

[KateBrodie]

Great! When I get to the hotel I will approve and merge into development!

On Sun, Jun 3, 2018 at 12:47 PM mpalmsten notifications@github.com wrote:

Thanks Kate for running the standard test. I appreciate you putting that together and I'm looking forward to learning more about it this week!

I addressed issues 1 and 3 from your review. I agree with your comment in 2 that we should run a check to make sure we have all inputs when we read in the params file (e.g., before we call analyzeBathyCollect.m). I think we should hold off until after the boot camp.

— You are receiving this because your review was requested.

Reply to this email directly, view it on GitHub https://github.com/Coastal-Imaging-Research-Network/cBathy-Toolbox/pull/38#issuecomment-394175074, or mute the thread https://github.com/notifications/unsubscribe-auth/AGbXyqoeJhHVwo_YbJUL0f2qqbBLhHPDks5t5BM7gaJpZM4UXfaE .

[RobHolman]

Just to be clear (cuz I'm a gitDunce), have we approved all of the bugs that came up? Also, it would be useful to have John comment on the approach, for example whether to include this in params. @mpalmsten @RobHolman @jstanleyx