Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Diagnostics tool for ill-posed constraints #1454
Diagnostics tool for ill-posed constraints #1454
Changes from 1 commit
37e0fae
1664593
4d48741
a1e91d2
33a4435
747eb7d
707de1e
3a6400b
c3e315c
2ab55ca
2af9478
b9d12ea
162f631
90d2cd3
152b044
81c1835
849647c
bff77d5
54e87c4
11d3f33
85d1956
d486e85
1048b32
d10a493
00a3434
d27de22
6d6fe2c
9dbd521
678caf8
47a58f4
1b5d1d2
18a6ff1
ef2e228
70636cb
5d5da12
c173984
7c03467
21cca53
ad6dd99
a3cde35
ddc8fb8
bd331e8
b3098e1
60e71cb
aebb118
9956286
8cf3bbe
34a2b58
93ef9d8
3284d3e
2d5ec93
5ffec52
161b15b
7deb640
86bd61c
9b13cb1
6577481
94fe8c0
360bb39
1fb2743
628f445
1a4a1ee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 I understand what you are doing here, but wouldn't this complain loudly about the objective for every parameter estimation problem (MSE) if the problem actually solved?
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.
One of my main concerns with this tool is its inability to distinguish between true cases of catastrophic cancellation and benign situations, where, for example, we have a heat of adsorption term in a column which will be close to except near the breakthrough point.
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.
Is there an easy way to test for that however? Note that this tool is only used for Cautions in the main toolbox, with the implication that these might be issues (but not guaranteeing that).
I.e., this is intended to be a simple check to find potential issues, but the user will have to look into them all further to decide if they are critical or not.
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'm not sure there's an easy way to do it without assuming a well-scaled system. In a well-scaled system, it would show up in the SVD with extremely large or extremely small singular values.
However, once you get to a high enough noise/signal ratio, you start to question about whether including the tool in
report_numerical_issues
is worthwhile or whether it is distracting the user from more fruitful avenues of diagnostics. I suppose we can just pull it fromreport_numerical_issues
without a deprecation cycle if it proves to not be useful, so we can try it out and see how users find 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.
@jsiirola This walker was written specifically with Constraints in mind, so I had not considered that. The check for mismatched terms would make sense for an Objective however, so this could be extended to handle them as well. However, how would the expression walker know if it was dealing with an Objective or a Constraint (the input argument is the expression, not the component)?
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.
Don't you need to negate the values for the second argument in the relational expression before treating it like a sum?
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 had forgotten to make that correction - thank you.
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.
Don't you need special handling for Ranged (like Equality / Inequality)? I would probably define a
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 I've fixed this - it would be good if you could double check my logic however.
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 have stumbled across a syntax that is a bit more concise. Not sure if you want to use it, though:
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 I'll keep the current form as it is a little easier to understand what it is doing.