Conversation
- force_stability -> force_sigma_in_rhp - some improvements
- type to type, subtype, parameter
No more operators dict in LTISystem, now (ss|si|os|oi)_operators dicts.
Member
|
Hi @pmli, I'm back at office, and looking at this PR is scheduled for Monday. Since this PR is open for a while: is there anything I should be aware of or is this ready to be merged? |
Member
Author
|
Hi @sdrave! There are a few things I can remember now which should be considered:
|
sdrave
requested changes
Nov 19, 2018
Member
sdrave
left a comment
sdrave
left a comment
There was a problem hiding this comment.
This is all looking very nice! Maybe we can manage to merge this by the end of the week? Most of my remarks are only minor issues. I will open a few additional issues for stuff to be done after the merge.
Codecov Report
|
This was referenced Nov 20, 2018
sdrave
approved these changes
Nov 21, 2018
Member
|
Ok, so shall we go ahead and hit the merge button? |
Member
Author
|
Ok, issue #478 is a bit tricky, I agree we should work on it after merging this. |
Member
|
So, let's do this! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Recent changes are mostly about simplifying the code. One notable change is the separation of
LTISystem.normintoLTISystem.h2_norm,LTISystem.hinf_norm, andLTISystem.hankel_norm. Now it's more clear that the only "bad" functions inLTISystemarepolesandhinf_norm, in the sense that they both need access to matrix data and they use algorithms of cubic complexity. Both of them now haveforce_denseargument, but there might be a better way of informing the user that these functions are "bad"...