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
Convert pid parameters to :parallel #900
Convert pid parameters to :parallel #900
Changes from 4 commits
a3b77eb
27a0c18
a43c256
692b5f9
c48eda3
9908eaf
5392ea1
0ac6a75
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 51 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L50-L51
Check warning on line 54 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L53-L54
Check warning on line 56 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L56
Check warning on line 60 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L59-L60
Check warning on line 62 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L62
Check warning on line 68 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L67-L68
Check warning on line 71 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L71
Check warning on line 74 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L74
Check warning on line 78 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L78
Check warning on line 82 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L81-L82
Check warning on line 85 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L85
Check warning on line 492 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L492
Check warning on line 523 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L523
Check warning on line 550 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L547-L550
Check warning on line 553 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L552-L553
Check warning on line 557 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L555-L557
Check warning on line 559 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L559
Check warning on line 575 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L575
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.
These lines are broken for parameter arrays
Check warning on line 580 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L577-L580
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.
Check warning on line 607 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L601-L607
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.
The
@.
here is not enough, the lines above, likewould fail for arrays
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.
Would you rather have Julia throw a
DomainError
here? In general to me it makes more sense to convert single pids at the time.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.
You could check
I agree, that would probably be a nicer implementation. I performed some tests locally, and it does not seem like most PID-related functions actually handle array parameters, and I cannot remember why it was supported in the first place. Maybe it's safe to assume that this functionality isn't used anywhere and not bother supporting 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.
Ah, I see now that
stabregionPID
does pass in array parameters, so if we drop support for array parameters this function would need an updateCheck warning on line 614 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L609-L614
Check warning on line 616 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L616
Check warning on line 626 in lib/ControlSystemsBase/src/pid_design.jl
Codecov / codecov/patch
lib/ControlSystemsBase/src/pid_design.jl#L625-L626