Implement MB02ED by saasaa · Pull Request #214 · python-control/Slycot

saasaa · 2023-09-19T18:38:08Z

Hi,

I have to implement the function MB02ED. Since I am no expert in Fortran, F2Py or numerics I am opening this draft PR to make sure, that I haven't overlooked something obvious or am not following the conventions of this library properly.

I have at the moment:

Created the F2Py Signature for MB02ED.
Created the wrapper function mb02ed in slycot/math.py
Written a test using the example data from examples , which passes pytest

I still have to

Convert the SLICOT Fortran documentation to a numpydocstyle docstring

which I plan to do over the course of this week.

Have I overlooked something?

Thanks

bnavigator · 2023-09-20T06:51:28Z

Hi @saasaa,

glad to see someone working on this.

Your seem to have activated some linter in your editor. Please try to avoid the format changes in unrelated functions and import statements at the head of the module files for this PR.

saasaa · 2023-09-20T10:11:12Z

I am using black as formatter by default, but I think I have removed all changes.

saasaa · 2023-09-21T14:07:18Z

I have added the docstring

bnavigator · 2023-09-22T07:25:08Z

Please see #200 and consider using copy in the wrapper for B and T

bnavigator · 2023-09-22T07:26:59Z

Don't worry about the current test failures of 15188ef , they look unrelated

saasaa · 2023-09-23T18:37:33Z

I have added tests for wrong input parameters and a negative definite matrix, the tests for the wrong parameters pass, but the test for the negative definite matrix gives me an TypeError: SlycotError.__init__() takes 3 positional arguments but 4 were given and I cannot figure out the cause.

bnavigator · 2023-09-23T19:11:46Z

I fixed the CI errors with setuptools_scm. Please rebase onto current master branch.

bnavigator · 2023-09-23T19:37:37Z

+    SlycotParameterError
+        :info < 0:
+            if INFO = -i, the i-th argument had an illegal value.


> with pytest.raises(expected_exception=SlycotParameterError, match="k must be >= 0") as cm: E AssertionError: Regex pattern did not match. E Regex: 'k must be >= 0' E Input: '\nif INFO = -i, the i-th argument had an illegal value.'

If you want to have more meaningful error messages (not mandatory);

Suggested change

SlycotParameterError

:info < 0:

if INFO = -i, the i-th argument had an illegal value.

SlycotParameterError

:info = -2:

k must be >= 0

:info = -3:

n must be >= 0

and so on

Otherwise just remove the info < 0 zero case and make sure the arg_list is in the correct order, then raise_if_slycot_error will generate a reasonable message. You will have to adjust the regex pattern in the tests then.

add tests

saasaa · 2023-09-23T20:20:05Z

I have added docstrings for the different SlycotParameterError, I have fixed the checks in the function signature, but the tests still fail with E _wrapper.error: (k>=0) failed for 2nd argument k: mb02ed:k=-1

bnavigator

Looks quite good now. A few minor nitpicks:

Co-authored-by: Ben Greiner <code@bnavigator.de>

bnavigator · 2023-09-24T14:07:03Z

Thanks a lot for the hard work and the patience with the reviews.

saasaa · 2023-09-24T14:10:44Z

Thank you accepting my PR!

bnavigator · 2023-09-24T14:12:29Z

Just a heads up @saasaa: You have worked from the master branch in your clone. It has now diverged from upstream. You have to reset your origin and local master branches to upstream/master and use a new "feature" branch, when you prepare your next contribution. (https://github.com/python-control/python-control/wiki/How-to-contribute-with-a-pull-request) Looking forward to it.

mb02ed

84c9552

saasaa marked this pull request as ready for review September 19, 2023 18:38

fix formatting

05fd0b7

added numpydoc style docstring

d373407

fixed typos in docstring

15188ef

bnavigator reviewed Sep 22, 2023

View reviewed changes