Remove duplicate calculations of ice impedance

[billsacks]

From @billsacks on September 15, 2017 12:38

@swensosc pointed out (a couple of months ago) that it would be good to remove duplicate calculations of ice impedance. These are things like:

10._r8**(-e_ice*top_icefrac)

10._r8**(-e_ice*(icefrac(c,1:3)))

and other variations on this. I think we can find all of these by searching for e_ice.

I did a quick search, and – while I think this is a worthwhile effort – I think it's going to take some effort. This is because there are many variations on this theme, and so some thought will be needed regarding how, exactly, to do this. I feel like it will take a full day or more to do this. So, given that this doesn't seem like the highest priority right now, I plan to defer this, unless others feel it should be done sooner.

Possible solutions I can see are the following - listed roughly in order from most efficient (but possibly hardest to accomplish) to least efficient (but possibly easiest to accomplish):

1. Calculate ice impedance just once each time through the run loop, and reuse that already-calculated value everywhere it's needed. However, I suspect that a single stored ice impedance variable isn't going to be sufficient, because of the many variations on the ice impedance calculation. So we may have to calculate a few versions of ice impedance to handle these different variations (also possibly using one of the other solutions below to remove duplication). Also, some analysis and/or testing is needed to see if it will work to just compute ice impedance once, near the start of the time loop, or if some code expects an updated ice impedance based on an updated icefrac (i.e., updated in the middle of the time loop).

2. Have an ice impedance routine that operates on arrays (within a filter). Then this could be called from the places that need it. This will require refactoring code that computes ice impedance, pulling the ice impedance calculation outside the loop.

3. Have an ice impedance routine that operates on single points. There already is such a routine in SoilWaterMovementMod, so this would need to be made public (either here or somewhere else). This will kill vectorizability if it's called from other modules, so only should be done as a last resort. However, it's possible that this could be combined with (1) without too much loss of efficiency - particularly if (1) involved just computing a few different versions of ice impedance all in a single module (in which case the single-point IceImpedance routine could potentially be inlined).

Copied from original issue: NCAR/clm-ctsm#18

[billsacks]

I am adding a new label, "DRY", for the common software engineering principle, "Don't Repeat Yourself" - since this is a common issue in CLM that I think will be needed on lots of issues.

[billsacks]

If we don't end up solving this problem, we should at least make 10 a named parameter in these equations, rather than a hard-coded and duplicated value.

[billsacks]

From @martynpclark on September 15, 2017 14:38

Hi @billsacks -- does it make sense to make 10 a named parameter since x=10._r8**(log10(x))? I'm not sure how these cases are handled in CLM. There can be other cases where the numbers in the equation are special, e.g., a quadratic function.

[billsacks]

does it make sense to make 10 a named parameter since x=10._r8**(log10(x))? I'm not sure how these cases are handled in CLM. There can be other cases where the numbers in the equation are special, e.g., a quadratic function.

I agree that it doesn't necessarily make sense to make it a named constant if it's 10 for this reason – and thus will never change. Is that indeed the reason why this is 10? (From just looking at the equations, it looked like just any old constant that might change later.)

[billsacks]

From @martynpclark on September 15, 2017 15:50

Yes, in this case it is a special number. I agree that sometimes such special numbers are difficult to identify just by looking at the equations.