search

schism-dev / schism-esmf

Earth System Modeling Framework cap for SCHISM
5 stars 6 forks source link

Restructuring NUOPC cap #30

Closed uturuncoglu closed 4 months ago

uturuncoglu commented 4 months ago

PR Description

This PR aims to restructure the existing NUOPC cap. Following items are implemented,

Git Issues Fixed By This PR

uturuncoglu commented 4 months ago

@platipodium @josephzhang8 Okay. I end up restructuring the existing cap (mostly schism_nuopc_cap.F90) to make it more flexible. It seems that the exiting reproducibility issue is solved completely. I tested with with both atm2sch and atm2sch2ww3 and they looks fine but I'll check it one more time by recreating the baseline on Hercules. Also, I need to work on little bit more since maybe I could solve the issue related with the outputting wave related fields (issue https://github.com/oceanmodeling/schism/issues/4). I have also some doubt about handling errors with usage of rc, and localrc and _SCHISM_LOG_AND_FINALIZE_ON_ERROR_. I could simplify it (get rid of having localrc) if you want but I don't want to do it before asking you. BTW, this is still draft and once I have confidence that everything is working as expected, I'll change its status.

platipodium commented 4 months ago

@platipodium @josephzhang8 Okay. I end up restructuring the existing cap (mostly schism_nuopc_cap.F90) to make it more flexible.

The restructuring is most welcome, I wonder whether I should also do that to the respective schism_esmf_cap.

platipodium commented 4 months ago

. I have also some doubt about handling errors with usage of rc, and localrc and _SCHISM_LOG_AND_FINALIZE_ON_ERROR_. I could simplify it (get rid of having localrc) if you want but I don't want to do it before asking you.

  1. My code convention was to use the keyword rc and the local variable localrcthroughout the code. But I don't mind if we replaced localrc with rc.
  2. There is always the complication with routines with optional return code, which is in my convention the local variable rc_ (with trailing underscore). That is a source of frequent error, so one must be careful
  3. What exactly is the issue with _SCHISM_LOG_AND_FINALIZE_ON_ERROR_ (Best create a new issue)? I could only speculate that it is both the input of a return code and the output?
uturuncoglu commented 4 months ago

@platipodium I might miss something in here but the routines return the localrc but there is no code to check the return value. In general, _SCHISM_LOG_AND_FINALIZE_ON_ERROR_ only checks the rc. Maybe I am wrong and this way if fine but we are generally checking rc and not using localrc in other caps. Anyway, this is totally up to you. If you want to keep like this that is fine since it is used like this in other part of the code.

I think at least every cap needs to have DataInitialize. Without it there is nothing to provide in the initial step. The model must run to fill its export state. With DataInitialize you could still provide some information through the export state like ocean mask and initial conditions to other components.

uturuncoglu commented 4 months ago

@platipodium BTW, probably I missed it but I might need to check other ocean components under UFS Coastal for their DataInitialize routines.

josephzhang8 commented 4 months ago

@uturuncoglu : in your change in esmf_util.F90, you checked the dimension against nea/npa to see the partition type (elem/node). This is not bullet proof, as in rare cases nea may = npa! Why not use

meshloc == ESMF_MESHLOC_NODE

In any case, if the elem partition is well tested, shall we remove node-based partition? Thx

uturuncoglu commented 4 months ago

@josephzhang8 This is the part that we are in ESMF_MESHLOC_ELEMENT and the ghost elements are removed. I put this because I would like to provide array to this routine on both nodes and elements. So, for example, I am providing ocean mask on elements but other variables such as currents etc. on nodes. It supports both. I see that it might create an issue for the cases that has nea = npa. Is it really possible even if ghost elements are removed? If each elements has 3 nodes then nodes will be more than elements. Maybe I am missing something in here.

platipodium commented 4 months ago

I see that it might create an issue for the cases that has nea = npa. Is it really possible even if ghost elements are removed?

Euler's rule says that np - ns + ne = C with C some small integer constant depending on the number of islands and topology of border. This theory binds the sum (np + ne) to be nearly equal to ns, but it does not provide any constraint on the relation np versus ne.

If the triangles are equilateral, each node is shared by six elements, and each element has three nodes, arriving at the usual ne = 2 * np, which we have in many real world cases. Here's ne and np for all meshes on my laptop, the least ne/np difference is 80/63.... dangerously close to np = ne?

1 3
72 49
80 63
108 130
192 115
572  336
1280 805
2304 1201
3153 1931
3328 1778
5000 2601
5780 3070
6400  3531
7944 4162
18110 11692
20480 10593
33586 17054
38960 20641
43009 27374
115600 58141
131508 69653
244863  126949
596398 305219
josephzhang8 commented 4 months ago

@uturuncoglu : yes it's theoretically possible since SCHISM allows quads. For pure quads, nea ~= npa

uturuncoglu commented 4 months ago

@josephzhang8 @platipodium Okay. Let me change the implementation. Probably I could pass an extra optional argument to this call. So, user could explicitly indicate the element based array is used as input. I could make node base as default.

uturuncoglu commented 4 months ago

@josephzhang8 @platipodium I updated the implementation. Now it does not relay on nea and npa comparison and user need to explicitly pass an argument to use element based array. It is bit-to-bit with previous implementation in UFS Coastal side. Let me know what you think. I also fixed minor issue that was preventing to compile the cap with GNU compiler.

josephzhang8 commented 4 months ago

Thanks, @uturuncoglu !