-
-
Notifications
You must be signed in to change notification settings - Fork 224
refactor: implement new additions to SII interface #2544
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
Merged
ChrisRackauckas
merged 4 commits into
SciML:master
from
AayushSabharwal:as/sii-default-values
Mar 13, 2024
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
e0fd18d
refactor: implement new additions to SII interface
AayushSabharwal e47afc2
test: add and update SII testset
AayushSabharwal b4a8b9c
format: docs
AayushSabharwal 713b2dc
fix: fix structural_simplify with variables marked as inputs
AayushSabharwal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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 might (almost certain) have unintended consequences.
input = true
does not have the same meaning as being part ofio
.Uh oh!
There was an error while loading. Please reload this page.
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.
for example, all
RealInput
in MTKstdlib are marked asinput = true
, but they should not be consideredio
unless explicitly requested by the user.Uh oh!
There was an error while loading. Please reload this page.
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 wasn't aware of this. The fix comes from a test failure:
input_output_handling.jl
expects the following system to be simplifiedHowever, without this change the test fails since
u
isn't recognized as an input. Should the test be updated?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.
hmm, I seem to have been a bit confused about how it works internally here, which test was it that failed?
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.
ModelingToolkit.jl/test/input_output_handling.jl
Line 53 in 539fe81
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.
ah, I see
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.
A few days ago, this code was actually being run unconditionally on
io == nothing
, so I don't think it will break anything. Also for MTKstdlib, it's probably more correct not run theinputs_to_parameters
unless io is provided, in case we want to make more use of theinput= true
metadata in the future. When the test was written, I thinkinput = true
was the only way to indicate an input, but since we added the io argument we changed approach to let the user-provided io indicate the externally visible inputs and outputs, while theinput = true
metadata lost most of its meaning. the reason is that whether something is an externally visible input is a property of how a component is used, rather than an inherent property of the component. Hence, specifying this information inside component does not make sense, and we instead pass io when we have assembled the whole systemThere 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.
#2547