moved the Forcing to its own IO#443
Conversation
|
The following tests FAILED: but the |
| void Forcing::readStreamIntoArrays() { | ||
| Error Err; | ||
|
|
||
| Real FillValueReal = -999._Real; |
There was a problem hiding this comment.
|
|
||
| std::string StreamName = "Forcing"; | ||
|
|
||
| // Attempt to read stream; if unavailable, log and fall back to zero forcing. |
There was a problem hiding this comment.
Does it fall back to zero forcing because all forcing tendencies will be computed and applied with zeros?
There was a problem hiding this comment.
yes, if the tendency flag is on but the forcing stream is missing, it defaults the forcing to zero and calculates the tendencies. If the tendency flag is off, then it skips the tendency calculation.
We can change that behavior later if we want a visible fail, the idea here was to write it in the log and proceed without failing.
There was a problem hiding this comment.
@cbegeman I suggested this; in order to produce as few breaking changes as possible. With us staring down the coupling deadline, I figure this might be worth it (though in generally it's not the best practice).
I think if we are OK with this for now, we should open up a linked issue about "failing fast" here. We can circle back to this after delivering a first pass at the coupled model.
I don't want to speak for @alicebarthel, but I don't think we are that tied to this approach. We are just trying to mindful of the work left and timeline. If people feel very strongly against defaulting to zeros, then I'd think we are okay with dropping this and just failing if the read is unsuccessful.
There was a problem hiding this comment.
Is there a check for whether any forcing tendency is on before we attempt to load the forcing stream? I.e., if we leave off the default-to-zero feature and the forcing stream is not provided, will the simulation always fail?
There was a problem hiding this comment.
There isn't a check on the tendency flag before the Forcing::init() or read from file.
Because the tendency flags are per forcing type, I didn't think of that since I expect most longer cases to have some forcing, but for the spin-down ones, we may be able to skip this altogether?
A check on whether at least one forcing flag exists is worth considering...
There was a problem hiding this comment.
I don't feel strongly that we need to run through all forcing flags before trying to read from the forcing stream. My concern with default-to-fill-value right now is that we may be introducing a requirement to provide a forcing file for all cases.
It could be good in the long run to have: if SfcStressForcingTendencyEnable is True, throw a critical error if SfcStressZonal and SfcStressMeridional are not included in the forcing stream (and do the same for other forcing fields and their corresponding flags).
I'm fine with default-to-zero for now as long as we open an issue that this needs to be fixed later.
There was a problem hiding this comment.
Agreed! I'll open an issue.
The key point of default to zero was indeed to avoid introducing a forcing file requirement for now.
|
OK, here is an update:
The ctests now all pass on pm-cpu (gnu). I typically copy these in my base directory the run in each case directory: At this stage, anyone testing this PR with ctests should also fetch these. |
| # and the surface restoring target files (if appl.). | ||
| Forcing: | ||
| UsePointerFile: false | ||
| Filename: forcing.nc |
There was a problem hiding this comment.
not saying we should do this, but could we set this to init.nc to delay the need for a polaris PR?
There was a problem hiding this comment.
I think this could be helpful for the tests that do not have any forcing. It would be weird to me to create an (empty?) forcing file for cases that wouldn't otherwise need one.
There was a problem hiding this comment.
I think it would be better to alter the logic so the forcing stream isn't read if it's not needed. Pointing to an unrelated file by default seems like a messy solution to me.
There was a problem hiding this comment.
Or to comment out the forcing stream by default.
There was a problem hiding this comment.
We could change the FreqUnits to never for now. And manually change to OnStartup for the few test we actually need it?
There was a problem hiding this comment.
That sounds ideal. @alicebarthel Could you use this approach in a companion Polaris PR and check that it works as expected?
There was a problem hiding this comment.
Yes, I like this approach. I'll look into it and check its behavior.
This PR updates the code to expect the forcing from a dedicated forcing IO (by Default forcing.nc)
A polaris PR is required to update the tests to read forcing from
forcing.ncinstead of theinit.nc, under development at https://github.com/alicebarthel/polaris/tree/update-forcing-ioChecklist
has passed, using the Polaris
e3sm_submodules/Omegabaseline