Convert misc formats from gio #7971
Conversation
lefticus
added
the
Refactoring
|
@lefticus it looks like this one should go in first, then #7971, both because this one is not a draft and also because of the PR number. There are some CI files showing diffs that look meaningful. Can you check those and either resolve the diffs or explain the reasoning? I'll do a review soon to get this going. |
|
Yes, this one needs to go first. I'm VERY confused by the diffs, as you can see that there were no diffs at all before my last merge from develop! |
|
@Myoldmopar I believe after my most recent merge from Develop we can call this one complete. |
| const auto cond_temp_error = format("'Cond Temp(C) = ' {:7.2F}", fmt::join(CondTempArray, " ")); | ||
| std::cout << cond_temp_error << '\n'; | ||
| ShowContinueError(cond_temp_error); | ||
|
|
||
| const auto curve_temp_error = format("'Curve Temp(C) = ' {:7.2F}", fmt::join(CurveValArray, " ")); | ||
| std::cout << curve_temp_error << '\n'; | ||
| ShowContinueError(curve_temp_error); |
There was a problem hiding this comment.
@lefticus The attached test files exercise the negative eir error message for both ChillerEIR and ChillerReformulatedEIR.
7971-GIOChillerError.zip
With v9.3.0 release, nothing is written to stdout (error messages like that never should be anyway). That said, the prior error message didn't work before (only one or two values were written to the err file).
The new code fixes it, but the writes to std::cout should be removed here and in the other chiller.
Also, in this one, remove the single quotes around Cond Temp(C) = in line 1456
And line 1460 should be "Curve Output ="
Thanks.
There was a problem hiding this comment.
OK, I'll remove those again. The old code definitely had something that was trying to write to stdout.
There was a problem hiding this comment.
@mjwitte does it make sense for GIOChillerError to become a regular test in the regression suite?
There was a problem hiding this comment.
@lefticus No, we try to keep the regression suite running with minimal errors. We could build a unit test for this. We have some coverage of errors in the unit tests, but it's a rather small percentage throughout the code.
There was a problem hiding this comment.
OK. It looks like I need to investigate a new failure on Windows and see if it's related to these changes. Hopefully we can get this PR wrapped up soon.
|
@mjwitte @Myoldmopar I just checked and these failures (mtd file, in Windows) are in an output file I have not yet touched, so I don't think it's related to this changeset. If either of you can confirm, I believe this PR is ready for merging. |
|
@lefticus correct, this error has been popping up randomly on Windows on other branches, not your problem. I am hopeful that once you wrap up all the output file changes then this strange access problem on the MTD file specifically will go away. That's not part of your scope, I'm just hoping it will be a nice side effect. |
Then I think it's ready for merge and we can get the next one in too shortly. |
|
It all looks good to me now, and CI is very happy. No need to dwell on it, merging. Thanks @lefticus. |
Convert many different formatting routines from GIO to {fmt}
Part of the ongoing refactoring process to remove GIO from EnergyPlus
0 changes expected.
Reviewer
This will not be exhaustively relevant to every PR.