-
Notifications
You must be signed in to change notification settings - Fork 15
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
Avoid values of baseY less than 0.001 #67
Conversation
This seems to indicate that the
|
@maltelenz I'm not sure I understand the point of your last comment. According to the analysis by @beutlich, this is the function that is actually called to compute the parameters of the tolerance tubes. If we don't change that function, the tubes will remain too tight for near-zero variables. As I said during the meeting, our current goal is not to restructure or refactor the CSV-compare source code, that would just take forever. Our immediate goal is just to avoid too narrow tubes when the variables are very close to zero when doing the current regression testing. Nothing more, nothing less. To my understanding, PR #66 just does that. Do you agree? |
One can try to make clean updates in the code that keeps the code understandable, or pay the ever increasing price of getting an increasingly unreadable code mess, making future understanding and fixes harder. Of course, sometimes it may be worth it to make a quick hack, and ignore future work and cost.
I don't know, I haven't spent the time to understand and test the change. |
This update simply swaps an epsilon of 1e-12 with an epsilon of 1e-3, which is more appropriate. I can't see how this could cause any future extra work or cost. Conversely, if we don't do this, we're stuck forever with regressions of the MSL 4.1.0. We need to move on with that. |
The line of code I quoted in my first comment, says what the
I'm not against changing things, I just thought I could share what I found about the |
I did some more analysis. The functions which calls either SetFormerBaseAndRatio() or SetStandardBaseAndRatio() are the constructors of TubeSize: csv-compare/Modelica_ResultCompare/CurveCompare/TubeSize.cs Lines 77 to 82 in 18b309d
csv-compare/Modelica_ResultCompare/CurveCompare/TubeSize.cs Lines 90 to 98 in 18b309d
I searched the entire code base for calls to such constructors, and I only found two of them, one here:
and one here csv-compare/Modelica_ResultCompare/CsvFile.cs Line 316 in 18b309d
The first constructor calls SetStandardBaseAndRatio(), the second constructor is only called with formerBaseAndRatio=true, so it only calls SetFormerBaseAndRatio(). To play safe, we can tweak both functions, just in case. I've updated the PR accordingly |
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.
@casella Thanks for the PR. Let me know if you are happy with my refactoring applied on top of your initial commit.
CurveCompare/CurveCompare.cs itself seems nowhere referenced. See #69. |
LGTM. Please merge it onto master |
This change considers a minimum range of Y reference values of 0.001, to avoid false negatives when handling near-zero values that are the result of balance equations affected by small numerical errors. 0.001 is a reasonable compromise, since the nominal attribute (which would be better) is not available in CSV result files.