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

Add map between MPAS-Ocean and Omega variables #237

Merged
merged 12 commits into from
Oct 21, 2024

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Oct 17, 2024

Checklist

  • Developer's Guide has been updated
  • API documentation in the Developer's Guide (api.md) has any new or modified class, method and/or functions listed
  • Documentation has been built locally and changes look as expected
  • Testing comment in the PR documents testing used to verify the changes

@xylar xylar marked this pull request as draft October 17, 2024 22:02
@xylar xylar requested a review from cbegeman October 17, 2024 22:02
@cbegeman
Copy link
Collaborator

Thanks for getting to this so quickly!

If I remember correctly, in our conversation yesterday we concluded that for IO streams we would just manually produce different sections in the yaml file for each model. Thus, we don't need a public function that has an MPAS-O var name as an input and the Omega var name as output. We would only be using these functions to rename initial state and output dataset variables.

@cbegeman
Copy link
Collaborator

From a brief look at the code, this looks good. Let me know when you would like me to test it, as I see this is marked draft.

@xylar
Copy link
Collaborator Author

xylar commented Oct 17, 2024

@cbegeman, I'm still working on this. I was about to write you to have a look but then I realized I'm not happy with it. I then revised it and I'm still not happy, but I'm getting there.

xylar added 2 commits October 17, 2024 18:07
This merge also adds or updates some attribute docstrings
@xylar
Copy link
Collaborator Author

xylar commented Oct 17, 2024

Okay, @cbegeman, this is now ready for you to take another quick look. The main difference from an hour ago is that the map_input_dataset() and map_output_dataset() methods don't require the calling code to worry about which model is being run. This should be more convenient, as it mean we can hopefully just call the methods right before writing datasets out or right after reading them in. If it looks okay, I'll update the manufactured solution test to use the new methods.

Comment on lines +1 to +6
variables:
temperature: Temp
salinity: Salt
tracer1: Debug1
tracer2: Debug2
tracer3: Debug3
Copy link
Collaborator Author

@xylar xylar Oct 17, 2024

Choose a reason for hiding this comment

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

Are we aware of others?

@xylar
Copy link
Collaborator Author

xylar commented Oct 18, 2024

Darn it! I realized immediately after trying to use this in manufactured_solution that this won't work because it's precisely for ocean steps that are not model steps that we need this mapping. I'll think about it some more and give it another go.

xylar added 3 commits October 17, 2024 20:20
This step is appropriate for most ocean steps that are not
OceanModelSteps, since often variables should be translated to
or from the native model names to MPAS-Ocean names for use in
Polaris
@xylar
Copy link
Collaborator Author

xylar commented Oct 18, 2024

@cbegeman, I now have a solution I'm happy with. I need to test it out, hopefully tomorrow.

In the meantime, let me know if it looks okay to you.

@cbegeman
Copy link
Collaborator

I think this looks great! I like the IOStep idea. Somehow the map_to_model_dataset and map_from_model_dataset names aren't super intuitive to me but given that we'll mostly be calling the read/write method it doesn't matter much

@xylar
Copy link
Collaborator Author

xylar commented Oct 19, 2024

Somehow the map_to_model_dataset and map_from_model_dataset names aren't super intuitive to me

@cbegeman, I'd appreciate suggestions.

@xylar
Copy link
Collaborator Author

xylar commented Oct 19, 2024

Somehow the map_to_model_dataset and map_from_model_dataset names aren't super intuitive to me

@cbegeman, how about map_to_native_model_vars() and map_from_native_model_vars()?

@xylar
Copy link
Collaborator Author

xylar commented Oct 19, 2024

Testing

I successfully ran the pr suite and the manufactured_solution test case on Chrysalis with MPAS-Ocean.

I set up the manufactured_solution test case with Omega and successfully ran the init/200km step. The forward/200km step fails because output.nc is not produced (as usual). I confirmed that SshCell occurs in initial_condition.nc instead of ssh, as expected.

@xylar xylar marked this pull request as ready for review October 19, 2024 18:48
@cbegeman
Copy link
Collaborator

@cbegeman, how about map_to_native_model_vars() and map_from_native_model_vars()?

Sure, that's clearer to me. Or map_dataset_from_model_vars

@xylar
Copy link
Collaborator Author

xylar commented Oct 21, 2024

@cbegeman, I could go either way. I guess I don't see either a obviously clearer so I'm tempted to stick with what I have.

Anything else you'd like to see in terms of code changes or testing before this goes in?

tracer1: Debug1
tracer2: Debug2
tracer3: Debug3
ssh: SshCell
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not actually sure this is correct. We will see once Omega is actually writing output.

@cbegeman
Copy link
Collaborator

Nothing more from me. Feel free to merge as-is. Thanks!

@xylar
Copy link
Collaborator Author

xylar commented Oct 21, 2024

Thanks @cbegeman!

@xylar xylar merged commit 39c06b7 into E3SM-Project:main Oct 21, 2024
5 checks passed
@xylar xylar deleted the map-mpaso-omega-variable-names branch October 21, 2024 21:19
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.

2 participants