Update to cohort insertion and sorting routines

[adrifoster]

Description:

Updates the cohort insertion (insert_cohort) and sorting (sort_cohorts) to be more in line with best-practices for linked list methods. Also makes them a bound method of the patch class.

Also adds some unit testing to demonstrate utility and functioning of methods.

Fixes issue #1313

Collaborators:

@billsacks @glemieux @ekluzek

Expectation of Answer Changes:

yes, will not be b4b because it fixes issue #1313, but I updated the existing sort_cohorts and insert_cohort to fix the issue and this PR is b4b with that branch

/glade/derecho/scratch/afoster/tests_0122-124517de/SMS_Lm6.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-Fates.C.0122-124517de_int

Checklist

Integrator

• FATES PASS/FAIL regression tests were run
• Evaluation of test results for answer changes was performed and results provided

Test Results:

CTSM (or) E3SM (specify which) test hash-tag: ctsm5.3.020

CTSM (or) E3SM (specify which) baseline hash-tag: ctsm5.3.020

FATES baseline hash-tag: sci.1.80.9_api.37.0.0

Test Output:

[billsacks]

This is a really valuable set of changes! Actually, there are (broadly speaking) three sets of changes here, all of which are very valuable: (1) Reworking InsertCohorts and SortCohorts to make them cleaner easier to understand; (2) Adding unit tests for these routines; and (3) Providing a structure for setting up synthetic patches for functional unit testing - a path that I think is going to pay off substantially in facilitating both functional unit testing and more traditional unit testing of FATES.

I have a few relatively minor suggestions, but overall this is excellent!

[ekluzek]

I really love seeing this come in! I've long been nervous that in FATES we have a complex data-structure (linked-lists) being used without unit testing built around it. This is such a critically important part of the FATES Software design that it really needs rigorous testing around it. It's also something that would be easy to go wrong, and without this kind of testing be really difficult to find later. And it could be something to give subtly wrong answers, and not have a clear way to find what went wrong. Those type of debugging problems are difficult to even detect, and very difficult to track down. They are intractable problems that you can't put time estimates to fix on. Having unit testing put in now though means three things.

1. We'll easily see when something breaks immediately
2. Finding the problem when it breaks will be more straight forward
3. So it will save multiple people days or weeks of debugging time down the line

Which is a huge benefit for FATES. I think it's hard to overstate how helpful bringing this is in will be. I'd class it as a game-changer for FATES development. It will allow us to refactor parts of the code that we couldn't before -- because we couldn't have confidence that the changes would be correct. So it allows the design of FATES to improve with time with more of an Agile Software Development Methodology workflow. That's really huge here.

Another benefit is that it adds some infrastructure that will allow doing similar kinds of unit testing easy to add in for other features. This is also a future time saver and so important to have in place.

So thanks to @adrifoster for getting this in for the FATES team.

I will confess that I only skimmed through the code, and didn't see anything obvious to point out. But, I'm grateful that @billsacks did an extensive careful review of the details. Also with the unit-testing I went through the unit-tests and thought if there's anything missing in the testing, and I didn't come up with anything obvious. I also have a lot of confidence here because I know the careful testing @adrifoster has been doing AND the unit testing itself gives a TON of confidence that's done correctly. Without that testing it would be harder to say it's OK.

Everything I'm saying here are just the reasons that unit-testing and, just better testing all around, is an industry standard best practice in the SE industry. It's something we need to shift to doing more of.

[adrifoster]

@billsacks and @ekluzek thank you so much for the review! I appreciate all the comments and suggestions. Hoping to get to those today and then will re-run testing.

[adrifoster]

@billsacks thanks I just finished bringing in all your requested changes! I am going to kick off regression testing

[billsacks]

I have looked through the changes addressing my more substantive comments and am happy with these changes. This looks great to me now! Once again, thank you for all of this valuable work supporting more unit testing in FATES!

[adrifoster]

fates testing on derecho all passes as expected (b4b against fixed branch):

/glade/derecho/scratch/afoster/tests_0131-100214de

fates testing on izumi running...

[adrifoster]

okay finally able to check izumi and it also passes!