E3SM-Project / polaris

Testing and analysis for OMEGA, MPAS-Ocean, MALI and MPAS-Seaice
BSD 3-Clause "New" or "Revised" License
7 stars 13 forks source link

Fixup partition file presence #225

Closed cbegeman closed 2 months ago

cbegeman commented 2 months ago

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

Checklist

cbegeman commented 2 months ago

Testing

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

PASS tests from all suites

cbegeman commented 2 months ago

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

cbegeman commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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

xylar commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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.