More work on NUOPC cap - DO NOT MERGE YET! #27
Conversation
src/schism/schism_esmf_util.F90
Outdated
| @@ -573,8 +579,17 @@ subroutine SCHISM_StateUpdate1(state, name, farray, kwe, isPtr, rc) | |||
|
|
|||
| if (intent == ESMF_STATEINTENT_IMPORT) then | |||
|
|
|||
| do ip = 1, isPtr%numOwnedNodes | |||
| farray(isPtr%ownedNodeIds(ip)) = farrayPtr1(ip) | |||
| ! all nodes that construct the element will get same value | |||
There was a problem hiding this comment.
Before accepting this change, we have to consider why we would project node vars onto elems. Shouldn't we rather just make sure that the size of farray is matching with the node / elem based vars and then based on meshloc info not project?
There was a problem hiding this comment.
@platipodium I placed if statements to the function to work differently in node vs element. I did not check the node version yet but I'll do. Please wait for my confirmation that node (with ESMf connector) is also working as expected before merging the PR.
|
@platipodium Thanks. I add couple of more things to fix the current. I'll look at your review tomorrow and fix the issues. Thanks again. |
There was a problem hiding this comment.
Thx @uturuncoglu . The changes look fine to me.
On another thread you seemed to imply you made changes to downtream. Shall we update ufs-coastal and redo RT? THx
@platipodium: can u plz review/approve. Thx
|
@platipodium I did changes for your review. The last thing is test the datm+schism configuration with ESMF connectors and also using meshloc node, which is default at this point. |
uturuncoglu
changed the title
|
@platipodium I checked the datm+schism case with connector and node based implementation. The model seems working but when I checked the import state I could see following weird patten,
As you can see, there is a region with full of zeros in the import state which also aligns with the decomposition elements. I also checked the model native output using VisIt and probably we have similar issue in there too,
This gets more smooth in the model output once the model progress in time. So, we might need to look at more carefully and if you would like to retain the node based approach, this needs to be fixed in the model side. Maybe this is can be fixed if you apply halo update after receiving ESMF fields (maybe you are already doing it, not sure). I could also try with the code version before we introduce the element based approach to eliminate any missing code to retain both approach but I think this was also exist but not exposed. To that end, Have you ever checked import and export states visually before. I am not sure but this might not be an issue in CoastalApp since in those configurations uses custom data atmosphere (ATMMESH) in there and it was always providing data with the same resolution of the model and using redist in the connectors. In our case, CDEPS data atmosphere is not in the same resolution with the model and we are using bilinear with NUOPC connectors. Other than this the PR seems good to me. Anyway, let me know how you want to proceed. |
|
@uturuncoglu I also recalled something similar (not same) to this, when we were playing with node based partitioning. Halo exchange was part of the problem and Robert O.'s solution seemed to solve the issue eventually. Anyway, my guess this is related now to the halo... thx |
|
@josephzhang8 Yes, it seems like that. Just to clarify, the last run that I mentioned is using node based partitioning along with the NUOPC connectors. So, this case must be same with the original code that supports only node based partitioning. Anyway, I'll look at more to find out any part that needs conditional (element vs. node). |
This Pr aims to update NUOPC cap to support configurations under UFS Coastal Model.