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

Fixup partition file presence #225

Merged
merged 11 commits into from
Sep 27, 2024

Conversation

cbegeman
Copy link
Collaborator

@cbegeman cbegeman commented Sep 23, 2024

Include graph partition file only when the ocean model is MPAS-Ocean so there aren't unnecessary files in Omega tests.

Checklist

  • User's Guide has been updated
  • 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

@cbegeman
Copy link
Collaborator Author

cbegeman commented Sep 23, 2024

Testing

PASS manufactured solution init and forward steps for MPAS-Ocean and Omega

PASS tests from all suites

@cbegeman cbegeman added framework Changes relating to the polaris framework as opposed to individual tests or analysis ocean Related to ocean tests or analysis labels Sep 23, 2024
@cbegeman
Copy link
Collaborator Author

@xylar Do you want to take a quick look and let me know if this is what you had in mind?

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

Looks good! I'd suggest a rename graph_filename --> graph_target to avoid confusion, see my detailed comments.


self.graph_target = graph_filename
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's also rename the parameter from graph_filename to graph_target to avoid confusion.

Then, self.graph_target doesn't need to be an attribute since it's not used except below to add the input file.

polaris/ocean/tasks/manufactured_solution/forward.py Outdated Show resolved Hide resolved
@cbegeman
Copy link
Collaborator Author

Looks good! I'd suggest a rename graph_filename --> graph_target to avoid confusion, see my detailed comments.

@xylar Sounds good! I'll make that change and retest

@xylar
Copy link
Collaborator

xylar commented Sep 24, 2024

@cbegeman, it looks like my suggestion (not having the graph_target attribute) won't work. I didn't realize it was getting used in the setup() method, I thought it was later in the __init__() method. Sorry for the confusion.

I don't think we want to add new parameters to the setup() method because that will cause extra burden on classes that descent from OceanModelStep() (they will always need a setup()` method). Let's go back to how you had it. Sorry about that!

I still think renaming the parameter to graph_target is important. And then the corresponding graph_target attribute to the class needs to be added to the doc string for the class.

@cbegeman cbegeman force-pushed the fixup-partition-file-presence branch from 0f3ae04 to ec75b9d Compare September 24, 2024 13:51
@cbegeman
Copy link
Collaborator Author

@xylar How about these code changes? It may be inconvenient sometimes to get the path from the base work dir rather than the relative path, but I think this is ok.

@cbegeman
Copy link
Collaborator Author

And yes, I'll fixup the doc strings once we settle on the code

@xylar
Copy link
Collaborator

xylar commented Sep 24, 2024

@cbegeman, yes, I think this could work. One piece is that the default value (in 3 places) of graph_target='graph.info' presumably doesn't make sense anymore. I think we have to provide graph_target the way things are written and it shouldn't have a default value anymore. But the alternative would be to set self.make_graph = True if graph_target=='graph.info' and then not try set the target parameter in that situation when you add graph.info as an input file. We can talk about this more when we meet shortly.

@cbegeman
Copy link
Collaborator Author

I noticed that the default value didn't make sense but I didn't think of the make_graph approach. Maybe in the case where we make_graph we make the default graph_target=None? Do we need the ability to specify the file name of the graph?

@xylar
Copy link
Collaborator

xylar commented Sep 24, 2024

Right, I think graph_target=None is the right default. And then we just need:

if self.graph_target is None:
    self.make_graph = True`

Adding the input file should work without further modification.

@cbegeman cbegeman marked this pull request as ready for review September 26, 2024 23:24
Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

@cbegeman, This looks great!

I've run a whole bunch of tests. I'm still working my way through the spherical tests but no sign of trouble so far.

update: all the spherical tests passed.

I also set up and ran manufactured_solution with Omega. It ran successfully but then gave an error (as expected) on the missing output.nc file. Also, as expected, there is no graph.info file or partition file in the forward directory.

@cbegeman cbegeman force-pushed the fixup-partition-file-presence branch from 9cc9899 to d284d33 Compare September 27, 2024 17:09
@cbegeman cbegeman merged commit a010381 into E3SM-Project:main Sep 27, 2024
5 checks passed
@cbegeman cbegeman deleted the fixup-partition-file-presence branch September 27, 2024 17:10
cbegeman added a commit that referenced this pull request Oct 10, 2024
Fixup graph paths for cached inputs

This PR corrects a small bug introduced by #225. When inputs are cached, we need to point to the mesh step rather than the init step for the graph files. Doesn't hurt to point to the mesh step when inputs are not cached either.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework Changes relating to the polaris framework as opposed to individual tests or analysis ocean Related to ocean tests or analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants