tracers in upwind transport

[eclare108213]

Upwind transport should use the tracer dependency arrays instead of doing calculations explicitly in work_to_state and state_to_work. Should we deprecate the upwind scheme?

[dabail10]

Does anyone use upwind? We should probably do a survey before getting rid of it.

[eclare108213]

This could be a discussion topic during the town hall -- whether to deprecate some things, and if so, what. Upwind and 0-layer thermo come immediately to mind. A general survey is an interesting idea. I suspect that there will always be someone who thinks we should keep things in the code. E.g. last fall one of our German colleagues strongly urged me not to get rid of the 0-layer thermo.

[phil-blain]

I just talked with JF about this. His opinion is we should deprecate upwind... @JFLemieux73

[eclare108213]

I tend to agree with @JFLemieux73.
@duvivier We could make this a discussion topic for the workshop or tutorial, maybe 30 min. This is the kind of feedback I'd like to get from users.

[duvivier]

@eclare108213 Yes, I'll start a list of specific topics you and I can discuss at we put together a draft agenda.

[JFLemieux73]

I know I said before we should deprecate upwind but I have changed my mind. I am involved in an intercomparison project where we do idealized experiments (similar to box2001). My colleagues just asked me to do a run with upwind...I think it would be a good idea to keep for this kind of idealized experiments.

[eclare108213]

@JFLemieux73 we can certainly keep the upwind functionality, but the issue with the tracers remains. I suggest that we revert the changes in #508 and make sure there is an abort if there are any tracers other than those that upwind is able to advect (e.g. by checking that ntrcr is not larger than the number of required tracers). I've updated the code deprecation list.

[apcraig]

I will create a PR to revert #508 and to add a check on ntrcr. Can someone clarify what that check should be? I will also try to figure it out.

[eclare108213]

Thank you @apcraig. I was thinking that we could just check that ntrcr is less than or equal to the number of required tracers. But looking at the upwind advection routines, it's more complicated than that. The issue lies in the routines state_to_work and work_to_state, which load and unload arrays to be advected. It looks like work_to_state is probably okay. In state_to_work, the question is whether all of the potential tracers are captured in the manually loaded works array. I don't think so, although many of them are. There is this code check at the end of that routine:
if (narr /= narrays) write(nu_diag,*) & "Wrong number of arrays in transport bound call"
which hopefully then aborts if something is missing. So maybe this is okay for now, although I'd still prefer that this code be rewritten so that works is not manually loaded. If the narr check is working, we can revert the deprecated code and the re-writing can continue to wait.

[apcraig]

I'm trying to understand the upwind, it looks like overall, fields are copied into works (state_to_work), then upwind_field is called, then works is copied back out to the fields (work_to_state). upwind_field seems to operate on each field, one at a time, and there is no dependence on the order of the fields or on trcr_depend. What is packed and unpacked are combinations of fields. work_to_state uses icepack_compute_tracers. state_to_work seems to do the opposite of work_to_state but has an implicit understanding of icepack_compute_tracers to do so. What I think we want is equivalent routine in icepack is that consistent with icepack_compute_tracers? Am I on the right track? We can take discussion offline if it makes sense.

Maybe we could revert the PR and then update the upwind implementation as a separate task to be done soon? I think that's probably cleaner anyway in terms of tracking changes.

[eclare108213]

You're on the right track.

Maybe we could revert the PR and then update the upwind implementation as a separate task to be done soon? I think that's probably cleaner anyway in terms of tracking changes.

Yes, I think this is the best approach. Then:

What I think we want is equivalent routine in icepack is that consistent with icepack_compute_tracers?

Maybe. This is done properly for the incremental remapping advection. If we do the same thing for upwind, then a new routine might makes sense, but it might be possible to just mimic what's done for remap using the existing routines (e.g. state_to tracers, etc).

[apcraig]

I saw state_to_tracer and tracer_to_state. If it makes sense to mimic those, I like that idea.

I will run the test suite on the reverted sandbox and then create a PR. Then I can play with the update of the upwind implementation and we can discuss further.

[eclare108213]

Now that upwind has be de-deprecated, we still have this tracer issue. @apcraig and I took a look at it earlier today, and the essential issue is how the tracer arrays are loaded into work arrays that are advected. Remap expects them in a different form than upwind does. I think it's fine to have separate routines to load them for each type of advection, but they should follow the same generic steps. It's not clear to me that upwind captures all of the tracer combinations, so the first thing to do is to compare upwind and remap runs in which as many tracers as possible are turned on, particularly brine height, zbgc, and zsalinity tracers. The tracer history output won't be BFB but should be qualitatively similar. If they are, then this present issue is not a bug but rather an opportunity for cleaning up the code, making it more internally consistent and less error-prone.