search

schism-dev / schism-esmf

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

Fix attribute warnings and misleading nsw message #29

Closed uturuncoglu closed 4 months ago

uturuncoglu commented 4 months ago

@platipodium @josephzhang8 If you don't mind please review this PR that fixes attribute issue that is mentioned in https://github.com/oceanmodeling/schism/issues/1 and also remove misleading message about nws option. Please also note that the similar fix might be also implemented for the SCHISM_MeshCreateNode. So, let me know if you want me to fix that one.

uturuncoglu commented 4 months ago

@platipodium Thanks. I wonder if these changes are tested in you side. If so, is it working correctly. If so, I could make changes also in SCHISM_MeshCreateNode. Also, noticed that these fields are added to export state due to satisfying requirements related with ugrid convention. Can you explain this to me little bit? Why we need these fields and attributes? Where are they used?

platipodium commented 4 months ago

Hi @uturuncoglu, I only tested successful compilation in ESMF-coupled system, I did not run it. The metadata requirements for ugrid can be found in the ESMF RefDoc section 12.8.4 CF Convention UGRID File Format. They are used in plotting tools such as psy-ugrid.

josephzhang8 commented 4 months ago

Dan tested some cases a while ago; @danishyo can u plz re-test with the latest version to see if u have errors (as @uturuncoglu mentioned). @uturuncoglu : can u tell Dan which test u had errors? Thx.

uturuncoglu commented 4 months ago

@danishyo @josephzhang8 the failed test is ike_shinnecock_atm2sch2ww3. For now I pointed the working hash in UFS Coastal side. So, once you checkout you need to point SCHSIM upstream head. In the meantime, I will look at reproducibility issue. Thanks for your help.

uturuncoglu commented 4 months ago

@platipodium @josephzhang8 Please test every PR coming from me. Otherwise, we could not be sure it is working or not. I am testing in my side but this is limited and I might did something wrong in my end. Without testing we could not ensure the stability and integrity of the component code which I think it is very important. There is no rush to merge without proper test which is totally fine for me. As a part of the UFS Coastal project we are planing to move implement GitHub testing for components. Like, datm forced with schism. We have component testing action in ESMF side and using it with noahmp model. If you want I could try to implement also for schsim and test UFS configurations.

uturuncoglu commented 4 months ago

@platipodium is there any application in schism side that uses ugrid? If not is this just for future use?

josephzhang8 commented 4 months ago

@platipodium @josephzhang8 Please test every PR coming from me. Otherwise, we could not be sure it is working or not. I am testing in my side but this is limited and I might did something wrong in my end. Without testing we could not ensure the stability and integrity of the component code which I think it is very important. There is no rush to merge without proper test which is totally fine for me. As a part of the UFS Coastal project we are planing to move implement GitHub testing for components. Like, datm forced with schism. We have component testing action in ESMF side and using it with noahmp model. If you want I could try to implement also for schsim and test UFS configurations.

@uturuncoglu : r u gonna be at OSM? We can discuss this there. Or, we should have a meeting on this. I think right now there is some confusion on all sides. Thx

uturuncoglu commented 4 months ago

@josephzhang8 No. It would be nice to discuss and learn more about the workflow in SCHSIM side. Let me know when you have time. Best