Skip to content
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

Fix nuopc cap for radiation stress components #1272

Conversation

uturuncoglu
Copy link
Contributor

Pull Request Summary

This PR aims to fix NUOPC cap (mesh cap) to enable coupling with ocean component via radiation stress components.

Description

Fixes NUOPC cap to correctly provide radiation stress components in export state.

Issue(s) addressed

Commit Message

N/A

Check list

Testing

  • How were these changes tested?
    The DATM+SCHSIM+WW3 configuration is tested and passing in UFS Coastal side
  • Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)
    Yes, we have a DATM+SCHSIM+WW3 RT under ufs-weather-model fork
  • Have the matrix regression tests been run (if yes, please note HPC and compiler)?
    Coupled cpld_control_p8 is running without any issue on Hercules with Intel compiler.
  • Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.)
    No answer change
  • Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):

@uturuncoglu
Copy link
Contributor Author

@JessicaMeixner-NOAA I am closing the draft PR - #1255. This is the minimal change required in the NUOPC cap to run two-way coupling between ocean and wave using radiation stress components.

Copy link
Collaborator

@JessicaMeixner-NOAA JessicaMeixner-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uturuncoglu Thanks for making this PR!

I have one quick question about the location of a few variables in relation to if-statements.

Do you have a ufs-weather-model PR that you plan to create along with this?

@@ -145,6 +145,9 @@ subroutine advertise_fields(importState, ExportState, flds_scalar_name, rc)
call fldlist_add(fldsFrWav_num, fldsFrWav, 'Sw_vstokes')
else
call fldlist_add(fldsFrWav_num, fldsFrWav, 'Sw_z0')
call fldlist_add(fldsFrWav_num, fldsFrWav, 'Sw_wavsuu')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uturuncoglu should this be in the else for the cesm coupled? or should this be completely outside of this if/else statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JessicaMeixner-NOAA I think those variables are not used by CESM but we are using under UFS Coastal. So, I did not want to change anything in CESM side by putting them outside of the if statement.

@uturuncoglu
Copy link
Contributor Author

@uturuncoglu Thanks for making this PR!

I have one quick question about the location of a few variables in relation to if-statements.

Do you have a ufs-weather-model PR that you plan to create along with this?

No, we have no UFS Weather Model level PR. This is isolated by WW3.

@JessicaMeixner-NOAA
Copy link
Collaborator

@uturuncoglu since this is going to dev/ufs-weather-model, we need a companion ufs-weather-model PR and the merge of this PR is dependent on the corresponding ufs-weather-model PR. This shouldn't change any answers so likely could be combined w/another PR at some point.

@uturuncoglu
Copy link
Contributor Author

@JessicaMeixner-NOAA So, maybe that PR could have just syncing WW3. Right? Do you want me open a PR in the ufs-weather-model side and link this one to it?

@JessicaMeixner-NOAA
Copy link
Collaborator

@uturuncoglu yes! That would be great if you could do that.

Copy link
Collaborator

@JessicaMeixner-NOAA JessicaMeixner-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is approved.

Tagging @alperaltuntas for awareness for CESM. However, there should be no impactful changes for you.

This PR will not be merged until it's it turns in the ufs-weather-model queue.

@@ -753,14 +756,14 @@ subroutine export_fields (gcomp, rc)
call CalcBotcur( va, wbcuru, wbcurv, wbcurp)
end if

if ( state_fldchk(exportState, 'wavsuu') .and. &
Copy link
Contributor Author

@uturuncoglu uturuncoglu Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JessicaMeixner-NOAA @alperaltuntas I think this section is not working as it is since it is using different variable names in if statement and get field pointer part of the code. I am not sure this type coupling is tested under CESM or not but if it is running under CESM, I'll be surprised. So, I am not expecting any issue also in the CESM side.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uturuncoglu okay let me know if you need anything from my side.

@uturuncoglu
Copy link
Contributor Author

@JessicaMeixner-NOAA @DeniseWorthen also found a type error in the output related to the radiation stresses. I added that fix to this PR.

@uturuncoglu
Copy link
Contributor Author

@JessicaMeixner-NOAA @DeniseWorthen Closing this too since we have new PR in WW3 side. #1291

@MatthewMasarik-NOAA
Copy link
Collaborator

@JessicaMeixner-NOAA @DeniseWorthen Closing this too since we have new PR in WW3 side. #1291

Thanks for this update @uturuncoglu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants