-
Notifications
You must be signed in to change notification settings - Fork 7
Bring new regression test/s for Duck and CICE coupled configurations #179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/coastal_app
Are you sure you want to change the base?
Conversation
|
@mansurjisan @yunfangsun JFYI, I fixed the issue with the generation baseline files dynamically. I just need to change So, it seems that data in some PET are not identical. I checked one of the file with NCAR's cprnc tool like following, and it seems that BTW, the test is little bit slow since it is checking lots of files. I am not sure what is the best way to do it (maybe we could have a merge step and then check but that requires running additional post-processing step through the rt.sh and requires some costomization) but we could look at later. |
|
@uturuncoglu: zcoor*.nc can have NaN's, e.g. below bottom and on dry spots. This output is strictly for post-processing so if it causes error, I suggest we turn it off in param.nml (iof_hydro(25)) |
|
@josephzhang8 Thanks for the clarification. It is good to know. But, it seems that the position of those NaNs are changing. We could turn it off in our end but you might want to double check in the SCHSIM side to be sure that there is no real issue. @mansurjisan I'll update the param.nml to remove that variable and recreate the baseline. Let me know what you think. |
|
@josephzhang8 JFYI, some output from cprnc, |
|
Thx. I'll consider adding init for this array... |
|
@josephzhang8 Yes, that would be great. Thanks. Even something small/large value (1.e-20, 1.e20) and adding mask attribute to that variable will work better than NaN. |
|
@mansurjisan The test is passing now. So, you could check in your end. |
|
@uturuncoglu : I've added init for zcoor* and tested. Plz pull the latest master from schism repo and let me know if it solves your problem. Thx |
|
Hi @uturuncoglu , Sorry for the delay. The Duck RT test passed on my end without any issues. I used your run directory as a baseline when comparing the results on my end. Also, I tried to compare the water elevation between my simulation and the baseline run. I tried to combine the outputs from both the baseline run and my run using SCHISM's I tried to generate a baseline using My run directories are: regression test output: Baseline: |
mansurjisan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good to me. The new Duck RT test case passed on my end .
|
@mansurjisan Thanks for checking. That is great. We are not saving local_to_global files since we do not have any way to call the utility and post process output before checking at this point. If individual files written by each processor are same with the baseline then we are fine. Are we planing to wait to bring other tests (i.e. coupled with WW3)? I am asking because I am not sure other Duck configurations are ready or not. |
|
I think we can add the ATM2WW3 configuration for Duck in the existing RT system, although it's actively going through finetuning by @yunfangsun |
|
@mansurjisan @yunfangsun If it is ready we could add to this PR. |
|
Hi @uturuncoglu - I will take a look at the setup of ATM2WW3 configuration for Duck, NC case . If everything works on my end, I can add that to this PR. |
|
@mansurjisan That is great. Thanks for your help. Let me know if you need anything from my end. |
|
Hi @uturuncoglu and @mansurjisan , For the ATM+WW3, we also need the dataocean for the exchange of sea level data to WW3, do we have a template for dataocean ? Thank you! |
|
@mansurjisan @yunfangsun let's schedule a meeting for it. Maybe next week. Just like previous one, we could try to define ATM+WW3 with data ocean tis time. BTW, how ATM+WW3 is running now. Is it using data ocean? Could you point me a run directory so, I could check it. |
|
Hi @uturuncoglu You can test And now the ATM+WW3 is using dataocean |
|
@yunfangsun okay. let me check. maybe we do not need to have call. I'll update you. |
|
@yunfangsun I am not sure this is intentional or not but I am seeing following in your docn.stream file,
Actually, msl is the mean sea level pressure and you are passing this to WW3 as sea surface height. I think that is wrong. Could you double check your configuration? |
|
@yunfangsun BTW, if you filled msl with the ssh (I am not sure about its source - if not era5 then create new file and name it correctly) then please create new data file or add new variable to the file. We would like to create these RTs for the external users and collaborators. When they use these as a reference to create their own cases, this would be very confusing for them. |
|
@yunfangsun BTW, the msl is constant globally but varying in time. Still need to have new file and name the variable accordingly. Anyway, @mansurjisan @yunfangsun I think having global input is fine for now but we need to update the duck cases to use subsetted forcing data over the region not global in the future. @mansurjisan I think you were working about the RT documentation. Do you have anything for DATM+SCHISM and DAMT+DOCN+WW3 cases. We could add them to the app level documentation. |
|
Hi @uturuncoglu , I put the sea level data to a era5 file, and ok I will make a new nc file with the variable name So_h to replace it. Thank you! |
|
Hi @uturuncoglu , The new water level file is |
|
@yunfangsun Thanks. That is great. if you create that data, please name the file accordingly and use correct metadata in it. This will prevent the confusion in the user level. Also, please provide me the details of the configuration, link/citation to the original source of the data. Again, I would like to put those to the documentation. |
|
@yunfangsun Sorry for extra work but this is still not correct. Please delete all attributes from the So_h variable and correct them (just having correct long name, unit and standard name would be enough). Also, we do not need to have wind data in this file. That is already in the datm file. |
|
@josephzhang8 Thanks for update. I'll look at updating SCHISM but still working on bringing these RTs. @SmithJos13 It seems that your PR (https://github.com/oceanmodeling/ufs-weather-model/pull/174/files) requires updated SCHISM. Will it work with v5.14 or you need additional changes in SCHISM side. Maybe you already push those to SCHISM repositories but I am not aware of it. |
|
@uturuncoglu I think some changes have been pushed to SCHISM, I would need to check the latest version. |
|
Let me know if @SmithJos13 needs to push to schism repo. You can put them to master, and I'll cherry-pick the changes to v5.14. |
I never see those changes. You might have pushed them to a fork, not official schism repo. |
|
@josephzhang8 @SmithJos13 Okay. We could figure out what we need when we try to bring CICE configuration. @SmithJos13 Let me know when you are ready and having successful run with data component and fully coupled configurations. So, we could start working on it. |
|
@uturuncoglu I'm almost ready. I'm just generating some boundary conditions for SCHISM so I can give the fully coupled Bering Sea domain a burn. If that runs and generates some ice when we are good to go! I suspect I should know tomorrow or early next week. |
|
@mansurjisan @yunfangsun new RT is ready and passing the test. Please give a try to |
|
Thanks, @uturuncoglu . I will give it a try on my end. Regarding the Duck regression test documentation, I have a version from last year, but it needs to be updated to reflect recent changes. I will work on it. |
|
Thank you @uturuncoglu , I will try it |
|
Hi @uturuncoglu , I have tested the atm+ww3 case The results is fine, it could be brought to the RT. |
|
@mansurjisan @yunfangsun Please let me know if we have still issue with this PR. Otherwise, I'll merge it soon. |
|
Hi @uturuncoglu, I believe @yunfangsun is planning to share another configuration for DATM+SCHISM+WW3. The one we’ve tested so far is the setup where the variables are passed through DOCN. |
|
From @yunfangsun
|
|
@SmithJos13 New CICE RT is ready and has a baseline. If you don't mind could you try to run and check the results in your end (it is configured to run 6-hours). You could run it using following command, Also note that you need to checkout this branch ( |
|
Hi @uturuncoglu , I was working on adding the Duck ATM2SCH2WW3 regression test and while working on it , i noticed the exisitng coastal_ike_shinnecock_atm2sch2ww3 is failing. The failing is happening for this branch this While running the rt.sh script, something that I found suspicious is that when the code compiles, it completes within a minute. but when I cloned
didn't find an error log in I am not sure what's causing this error. This is my run directory on hercules: |
|
The issue appears to be originating from here. ufs-weather-model/cmake/configure_apps.cmake Line 158 in 8e72774
Since CSTLSW is not defined here, Cmake is not enabling WW3 during the build process. However, in the cmake file in ufs-weather-model/cmake/configure_apps.cmake Line 154 in 0a70fc3
I fixed it on my end for |
Commit Queue Requirements:
Description:
Commit Message:
Priority:
Git Tracking
UFSWM:
Sub component Pull Requests:
UFSWM Blocking Dependencies:
Documentation:
Changes
Regression Test Changes (Please commit test_changes.list):
Input data Changes:
Library Changes/Upgrades:
Testing Log: