search

schism-dev / schism-esmf

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

More work on NUOPC cap - DO NOT MERGE YET! #27

Closed uturuncoglu closed 5 months ago

uturuncoglu commented 8 months ago

This Pr aims to update NUOPC cap to support configurations under UFS Coastal Model.

uturuncoglu commented 8 months ago

@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.

uturuncoglu commented 7 months ago

@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 commented 7 months ago

@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,

Screenshot 2023-11-06 at 4 23 56 PM

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,

Screenshot 2023-11-06 at 4 32 36 PM

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.

josephzhang8 commented 7 months ago

@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

uturuncoglu commented 7 months ago

@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).