-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
fix: change variable defaults to guesses #281
Closed
avinashresearch1
wants to merge
7
commits into
SciML:main
from
avinashresearch1:avi/defaults_to_guesses
+57
−35
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
537e656
fix: change variable defaults to guesses in Thermal
avinashresearch1 b2f848c
fix: change certain defaults to guesses in Blocks
avinashresearch1 e6e880b
fix: revert SISO to function syntax instead of mtkmodel syntax
avinashresearch1 e7ac1e1
fix: syntax bug fix
avinashresearch1 2781924
fix. Fix typo
avinashresearch1 d2490b8
fix: Bug fix to have zero instead of zero vector
avinashresearch1 2ff740c
fix: change MIMO to use RealInput/OutputArray
avinashresearch1 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 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 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 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 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
Oops, something went wrong.
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.
I also had the same suggestion in #292, but #292 (comment)
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.
Most of the time this is what users want, but it is occasionally useful to have the system solve for an integrator value that makes an initial control signal have a particular value to avoid initial transients, so forcing the user to specify the initial state explicitly might not be too bad
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 guess the user can not easily remove a default value that has been specified in the library? If they cannot remove the value, we must not provide it since then it cannot be solved for
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.
Since we now have the capability to remove a default, the explicit state variables, like those in
Integrator, Derivative, FirstOrder, SecondOrder
should keep their defaults, buty,u
in SISO etc. should notThere 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.
Yes, I am probably going to make a new PR, since almost everything in this is not correct :(
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.
@ChrisRackauckas We can probably close this without merging since this PR has more incorrect than correct things, I'll make a new PR for the things that I would like to fix and the docs.
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.
okay