-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
More t is now a parameter test #989
Conversation
@@ -436,7 +436,8 @@ let | |||
end | |||
|
|||
@parameters k k2 | |||
@variables s x D(x) E(s) F(s,x) | |||
@parameters s x |
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.
Generally I did not merge the new independent-variable-paraemter declarations with old parameter declarations. It seems desirable to create a @independent_variables
declaration or similar for these in the future, so better keep them separate for when that happens.
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.
Sounds like that macro will happen, so we can update for it once available. For now we just want to ensure we don't get test failures or such from the change (and even if they change it to just be a warning now it would be good to avoid warnings in the test logs).
@@ -972,7 +975,8 @@ end | |||
# Checks that various erroneous coupled system declarations yield errors. | |||
let | |||
@parameters p1 p2 | |||
@variables τ U1(τ) V1(t) |
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.
This causes a a @test_throws
to go from throwing an error to not throwing an error (hence causing test failure). The reason is not directly obvious, but should be possible to figure out since I've been able to look closer at 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.
Interesting.
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.
Feel free to merge when you are happy (and I guess make a 14.0.2 release to get the docs updated).
Approved conditional on the test failure getting fixed that is... |
Primarily in the test files. These were the last cases I could find, but there might possibly be more that I missed. Need to read through tomorrow as well to check it looks OK.