E3SM-Project / Omega

Next generation ocean model within E3SM
https://docs.e3sm.org/Omega/omega
Other
4 stars 5 forks source link

Omega/state time level change #152

Closed sbrus89 closed 2 weeks ago

sbrus89 commented 1 month ago

This PR changes the time level interface in the State class to be consistent with what has been implemented in Tracers. The new functions getLayerThickness and getNormalVelocity retrieve the device array corresponding to a given TimeLevel. getLayerThicknessH and getNormalVelocityH provide the same functionality for the host arrays. All time index calculations for a given TimeLevel are done in the function getTimeIndex. A getTimeIndex function has been added to the Tracers class as well.

Checklist

brian-oneill commented 3 weeks ago

Changes from #148 need to be merged in and the tracer time level indices have to be updated in the doStep functions for each time stepper, e.g. https://github.com/E3SM-Project/Omega/blob/5d6af4699da45c869e7131c2185b8e9e618bd4db/components/omega/src/timeStepping/ForwardBackwardStepper.cpp#L15-L23

The separate TrCurLevel and TrNextLevel variables can be removed and just replaced with CurLevel and NextLevel in each function.

sbrus89 commented 3 weeks ago

Testing

Passes CTests on Frontier GPU

sbrus89 commented 3 weeks ago

As for keeping separate CurTimeIndex variables for State and Tracers, I think there may be advantages to managing them separate for spilt timestepping schemes (for example if a State instance were to be used in the barotropic sub-cycling), but would be curious what @mwarusz's and @mark-petersen's thoughts are on that.

grnydawn commented 3 weeks ago

@sbrus89, after further consideration, I also think that keeping a separate CurTimeIndex would be better. Changing the CurTimeIndex in either State or Tracers might require the other to not only update the variable but also update the pointers to Field as well.

As for keeping separate CurTimeIndex variables for State and Tracers, I think there may be advantages to managing them separate for spilt timestepping schemes (for example if a State instance were to be used in the barotropic sub-cycling), but would be curious what @mwarusz's and @mark-petersen's thoughts are on that.

sbrus89 commented 3 weeks ago

That's a good point, @grnydawn.

philipwjones commented 2 weeks ago

@grnydawn @brian-oneill @sbrus89 Is this ready to merge yet? Have all the review changes been made?

sbrus89 commented 2 weeks ago

@philipwjones, I've made all the review changes requested so far, and rebased to fix conflicts with #147.

brian-oneill commented 2 weeks ago

I just re-ran unit tests on Chrysalis and Perlmutter CPU & GPU, and everything passed. Looks like everything's been addressed, so I'll approve.

grnydawn commented 2 weeks ago

@sbrus89, @philipwjones, One item discussed during the stand-up meeting was that we may want to use a C++ enum class to specify CurTime, NextTime, and PrevTime, etc., so that users do not need to know the internal details of the index numbering scheme. Since this item has not been discussed among the reviewers here, I wonder if we should consider implementing (or discarding) it in this PR.

sbrus89 commented 2 weeks ago

Sorry, I missed that discussion on Monday @grnydawn . I'm open to adding that, but I think now the time level numbering convention is intuitive since it corresponds with the t^{n+1}, t^n, t^{n-1}, etc that are used in writing out the timestepping schemes.

philipwjones commented 2 weeks ago

While I don't like having magic constants around and would prefer named levels, I'm not sure how we would want do that for arbitrary number of time levels (though I suspect it's unlikely we'd have more than 2-3). And we'd have to have similar constants for tracers and any other group that has its own time levels and that might add more confusion. Let's think about it more and go ahead with the unnamed indices for now.