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.

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

implement changes from PR #157 to fix lqcontrol docstring #297

Merged
merged 2 commits into from Apr 20, 2017

Conversation

mmcky
Copy link
Contributor

@mmcky mmcky commented Apr 12, 2017

This PR

the only variation is to continued use of w_path rather than w_t used in PR #157

@mmcky
Copy link
Contributor Author

mmcky commented Apr 12, 2017

@sglyon. These changes were failing due to change in return object. Previously this method was returning Cw_path. Should this method return w_path as per docstring or Cw_path. This PR implements your previous fix.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 83.04% when pulling 5ca783a on fix-lqcontrol-doc into 758620a on master.

@mmcky
Copy link
Contributor Author

mmcky commented Apr 12, 2017

Based on this comment

I think it is still meaningful. In the docstring we claimed to be returning the w_t, but we were actually returning C * w_t

Also the number of elements in the return value was incorrectly documented.

This is now correct. I have added in the dot(c, w_t) step in the test.

@sglyon
Copy link
Member

sglyon commented Apr 12, 2017

Hey @mmcky Sorry but I don't understand the question. Can you repeat?

@mmcky
Copy link
Contributor Author

mmcky commented Apr 12, 2017

Hey @sglyon. The comment above came from PR #157 - which was a PR you made to implement the original change. I intend to close PR #157 and merge this PR instead after review.

@mmcky
Copy link
Contributor Author

mmcky commented Apr 19, 2017

@sglyon Just to clarify the question.

Should this method return w_path as per docstring or Cw_path. Cw_path is dot(C, w_path)

The tests on the previous PR was failing because the test assumed the function was returning Cw_path and not w_path as per the docstring for the function.

This PR implements your previous fix that never got merged.

@sglyon
Copy link
Member

sglyon commented Apr 20, 2017

Given that the user will already have C inside the LSS instance and we can't really undo the dot, my preference would be to return w_path.

@sglyon sglyon merged commit 7113f1b into master Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants