-
Notifications
You must be signed in to change notification settings - Fork 6
moved the Forcing to its own IO #443
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
base: develop
Are you sure you want to change the base?
Changes from all commits
ce219d6
cac6985
ffafd89
8d996bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
|
|
||
| #include "Forcing.h" | ||
| #include "Field.h" | ||
| #include "IOStream.h" | ||
| #include "Logging.h" | ||
| #include "Pacer.h" | ||
|
|
||
|
|
@@ -58,6 +59,7 @@ Forcing *Forcing::create(const std::string &Name, const HorzMesh *Mesh, | |
| } | ||
|
|
||
| // Initialize the default forcing instance and read configuration. | ||
| // Reads the forcing fields from streams at startup (for now). | ||
| void Forcing::init() { | ||
| if (DefaultForcing != nullptr) { | ||
| return; | ||
|
|
@@ -78,6 +80,10 @@ void Forcing::init() { | |
|
|
||
| Config *OmegaConfig = Config::getOmegaConfig(); | ||
| DefaultForcing->readConfigOptions(OmegaConfig); | ||
| // for now, forcing fields are read at start-up only. | ||
| // to be extended to include switch from standalone to coupled. | ||
| // to be moved to a Forcing->prepareForStep(SimTime) method later. | ||
| DefaultForcing->readStreamIntoArrays(); | ||
| } | ||
|
|
||
| // Return the default forcing instance. | ||
|
|
@@ -159,4 +165,32 @@ I4 Forcing::exchangeHalo() const { | |
| return Err; | ||
| } | ||
|
|
||
| // Read forcing fields from input stream. | ||
| // To be extended with time indexing later. | ||
| void Forcing::readStreamIntoArrays() { | ||
| Error Err; | ||
|
|
||
| Real FillValueReal = -999._Real; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| deepCopy(SfcStressForcing.ZonalStressCell, FillValueReal); | ||
| deepCopy(SfcStressForcing.MeridStressCell, FillValueReal); | ||
|
|
||
| std::string StreamName = "Forcing"; | ||
|
|
||
| // Attempt to read stream; if unavailable, log and fall back to zero forcing. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it fall back to zero forcing because all forcing tendencies will be computed and applied with zeros?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There isn't a check on the tendency flag before the Forcing::init() or read from file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I'm fine with default-to-zero for now as long as we open an issue that this needs to be fixed later.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed! I'll open an issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great. Glad we're on the same page |
||
| Err = IOStream::read(StreamName); | ||
| if (Err.isFail()) { | ||
| LOG_INFO("Forcing: Error while reading {} stream, using zero forcing", | ||
| StreamName); | ||
| deepCopy(SfcStressForcing.ZonalStressCell, 0._Real); | ||
| deepCopy(SfcStressForcing.MeridStressCell, 0._Real); | ||
| } | ||
|
|
||
| I4 HaloErr = exchangeHalo(); | ||
| if (HaloErr != 0) { | ||
| ABORT_ERROR("Forcing: Error exchanging halo for startup forcing fields"); | ||
| } | ||
|
|
||
| computeAll(); | ||
| } | ||
|
|
||
| } // namespace OMEGA | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not saying we should do this, but could we set this to
init.ncto delay the need for a polaris PR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or to comment out the forcing stream by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could change the
FreqUnitstoneverfor now. And manually change toOnStartupfor the few test we actually need it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that's the right solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I like this approach. I'll look into it and check its behavior.