iframe-proxy

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

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

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