Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add sb10yd #203

Merged
merged 11 commits into from
Aug 26, 2023
Merged

Add sb10yd #203

merged 11 commits into from
Aug 26, 2023

Conversation

KybernetikJo
Copy link
Contributor

@KybernetikJo KybernetikJo commented Aug 1, 2023 •
edited
Loading

This Draft/PR adds the SLICOT Function SB10YD.f.
A merge would close the issue #153.

It's working, following thing could be improved::

  • Slightly improve docstring.
  • Slightly improve unittests.
  • The order of the procedures must be restored.

@KybernetikJo KybernetikJo marked this pull request as ready for review August 3, 2023 16:12
@KybernetikJo KybernetikJo mentioned this pull request Aug 8, 2023
Open
10 tasks
@KybernetikJo KybernetikJo changed the title Add sb01yd Add sb10yd Aug 17, 2023
@KybernetikJo
Copy link
Contributor Author

@bnavigator

How I get your force-push into my local and origin branch?
I think I've done double work here.

@bnavigator
Copy link
Collaborator

It's alright, you did good. My force push was just a revert to (your) ddb8c03 because my attempt to resolve the merge conflict in 86032c8 had a mistake.

If there is a relevant change to your origin, you must merge or rebase your origin into your local before proceeding to push to origin.

bnavigator
bnavigator reviewed Aug 24, 2023
View reviewed changes
slycot/synthesis.py
mn = min(2*lendat,2*n+1)
if n > 0:
lw3 = 2*lendat*(2*n+1) + max(2*lendat,2*n+1) + max(mn+6*n+4,2*mn+1)
elif n == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please cover this case in the unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to look into that.

@bnavigator
Copy link
Collaborator

Please rebase your feature branch onto master, there are some duplicate commits now

@KybernetikJo
Copy link
Contributor Author

Yup, I have done something wrong.
I will try to fix it.

@bnavigator
Copy link
Collaborator

I fixed some linting warning in the tests, mainly in order to trigger a new CI run with a new commit. Somehow 6ff1902 was not picked up by GitHub Actions.

bnavigator
bnavigator approved these changes Aug 26, 2023
View reviewed changes
Copy link
Collaborator

@bnavigator bnavigator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bnavigator bnavigator merged commit 60a5c71 into python-control:master Aug 26, 2023
@bnavigator bnavigator mentioned this pull request Aug 26, 2023
Closed
@bnavigator bnavigator added this to the 0.6.0 milestone Aug 26, 2023
@KybernetikJo KybernetikJo deleted the add_sb10yd branch January 6, 2024 19:59
@roryyorke roryyorke mentioned this pull request Jul 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnavigator bnavigator bnavigator approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@KybernetikJo @bnavigator