docstrings changed for solve to match opty #301
Conversation
|
The docstrings need to follow the numpydoc format, see here: https://numpydoc.readthedocs.io/en/latest/format.html |
|
I would recommend copying the cyipopt docstring and then edit that so that the formatting and information match. We don't really want to remove any info that is in the cyipopt docstring, but just change the parts that are needed for opty: https://github.com/mechmotum/cyipopt/blob/master/cyipopt/cython/ipopt_wrapper.pyx#L636 |
|
I did copy - but from the part of the opty documentation instead of from the code directly. :-( |
|
It doesn't look like you copied this: https://github.com/mechmotum/cyipopt/blob/master/cyipopt/cython/ipopt_wrapper.pyx#L636 You are also missing the option arguments to solve. |
|
I did copy it. I did not push it, yet, because I am unsure about my point 2 above. |
|
Now I copied the docstring directly from the cyipopt code, and change the parts to make the opty docs consistent. |
opty/direct_collocation.py
Outdated
|
|
||
| """ | ||
|
|
||
| return super().solve(free) |
There was a problem hiding this comment.
This line is not correct because it will not handle the optional keyword arguments.
opty/direct_collocation.py
Outdated
| @@ -197,6 +197,54 @@ def __init__(self, obj, obj_grad, equations_of_motion, state_symbols, | |||
|
|
|||
| self.obj_value = [] | |||
|
|
|||
| # this is added only to correct the docstring, which is a bit different with | |||
| # opty compared to cyipopt. | |||
| def solve(self, free): | |||
There was a problem hiding this comment.
This signature does not match cyipopt's.
opty/direct_collocation.py
Outdated
| @@ -197,6 +197,54 @@ def __init__(self, obj, obj_grad, equations_of_motion, state_symbols, | |||
|
|
|||
| self.obj_value = [] | |||
|
|
|||
| # this is added only to correct the docstring, which is a bit different with | |||
| # opty compared to cyipopt. | |||
There was a problem hiding this comment.
These two lines are not need. If is common to overwrite methods in sub classes for this reason and it needs no explanation.
opty/direct_collocation.py
Outdated
|
|
||
| """ | ||
|
|
||
| return super().solve(free, lagrange=[], zl=[], zu=[]) |
There was a problem hiding this comment.
| return super().solve(free, lagrange=[], zl=[], zu=[]) | |
| return super().solve(free, lagrange=lagrange, zl=zl, zu=zu) |
Needs that changed, otherwise the arguments that the user passes in will never reach this method.
There was a problem hiding this comment.
Shows my weakness in python - after so many years! :-(
Corrected as per suggestions.
Added a function solve to direct_collocation as suggested by JM above to get docstrings which better match opty.
In my tests in ran fine.