ESCOMP / CISM-wrapper

Community Ice Sheet Model wrapper for CESM
http://www.cesm.ucar.edu/models/cesm2.0/land-ice/
Other
3 stars 15 forks source link

Should we use different names for the different uses of "instance" in cism-wrapper? #66

Closed billsacks closed 2 years ago

billsacks commented 2 years ago

@whlipscomb @Katetc @gunterl - I'm interested in your feedback on this: We currently use the word "instance" in two ways in cism-wrapper: (1) to refer to different ice sheets (Greenland vs. Antarctica); and (2) to refer to different members in a multi-instance (ensemble) run, e.g., as is used for data assimilation. (1) is consistent with the use of "instance" in CISM, and (2) is consistent with the use of "instance" in other parts of CESM. It is unfortunate that the same word is used in both cases.

I have tried to add comments making it clear which meaning of "instance" is meant in different places, but I can see some potential for this to be confusing long-term. Please let me know if you feel it's important that I change the names of one of these uses, and if so, which one you would suggest changing and any ideas you have for a new name.

(My gut feeling is that, if we changed the name of either of these, we should probably change the name of (2), since (1) is used throughout CISM. Then we'd probably want an obvious comment - e.g., in source_glc/glc_ensemble.F90 - mentioning this name change from the rest of CESM.)

whlipscomb commented 2 years ago

@billsacks – Thanks for pointing out this potential confusion. I think it would be highly invasive to remove 'instance' from everywhere it appears in Glad and cism-wrapper. If the two names were mixed up within single modules, I'd probably opt for referring to different ensemble members as "members" instead of instances. But if the second usage is limited to a single module, glc_ensemble.F90, then I think we're probably OK with the comments you've already written there.

billsacks commented 2 years ago

In order to see how problematic this is, I did a quick search through the code for "inst". Here are the uses I see:

Other than that, instance refers to separate ice sheets.

It looks like, for the most part (maybe not 100% consistently, but close to it, and maybe 100%), variables with the full word "instance" in their name refer to different ice sheets, whereas variables with the abbreviated "inst" refer to these multi-instance ensemble members. I have added a note about that in glc_ensemble.F90. Comments are less consistent: in comments, "instance" can refer to either, but hopefully this is usually clear from context.

So it's maybe not too bad; while it's slightly worse than being constrained to a single module, it's not that much worse than that. If others are okay with this, I'll close this as a wontfix... but let me know if you feel it should be fixed based on this extra information. My overall sense is: if someone comes into the code realizing that there are these two uses of the word "instance", it probably isn't too hard to tell which use is being referred to in each place; the main confusion would arise if someone only knows about instance in the multi-instance sense or only knows about instance in the CISM multiple ice sheet sense. Hopefully then they would stumble upon one of my handful of comments pointing out those different uses.

whlipscomb commented 2 years ago

@billsacks, it's good that we have a distinction between "instance" and "inst". Given that the overlap is minimal and you've added comments, I'm OK with a wontfix. Thanks!