Change analysis.py to numpydoc style

[KybernetikJo]

This PR refactors the Slycot/slycot/analysis.py in order to be closer to the numpydoc style as discussed in #100.
Should be part of #204.

(finished 0 of 18 routines)

Routine name	Finished	Status description
ab01nd		*3
ab05md		*3
ab05nd		*3
ab07nd		*3
ab08nd		*3
ab08nz		*3
ab09ad		*3
ab09ax		*3
ab09bd		*3
ab09md		*3
ab09nd		*3
ab13bd		*3
ab13dd		*3
ab13ed		*3
ab13fd		*3
ab13md		*3
ag08bd		*3

Status description:

1. numpydoc docstring sections, fixed
2. parameters data types, fixed and 1 fixed
3. optional and default values, fixed and 2 fixed
4. ...., fixed and 3 fixed

At least every slycot routine should have the following numpydoc docstring sections

• Parameters
• Returns
• Raises or Warns

Scipy often uses the numpydoc docstring sections

• Parameters
• Returns
• See Also
• Notes
• References
• Examples
• (see scipy.signal.place_poles or scipy.linalg.solve_continuous_are)

Some parts may be related to the final decision on #199, #200.

• Behavior of intent(in,out) vs intent(in,out,copy) must be investigated again. (see discussion Bugfix ab13bd #200)
• Parameters: ndarray or array_like, depending on intent(in,out) vs intent(in,out,copy)?
• Returns: ndarray or array_like, depending on intent(in,out) vs intent(in,out,copy)?
• numpydoc, array_like "array_like : For functions that take arguments which can have not only a type ndarray, but also types that can be converted to an ndarray (i.e. scalar types, sequence types), those arguments can be documented with type array_like."

Further, discussing is needed.

[KybernetikJo]

I have done the main refactoring, which includes mainly:

• numpydoc sections (------)
• name : data types (A : array_like)
• optional and default values,

some default values are still missing, though.

---

I have added the lines in every routine

Raises
------
SlycotParameterError
     :info = -i: the i-th argument had an illegal value;

but I am not completely sure about that.

In
https://github.com/KybernetikJo/slycot-examples/tree/main/unstable/slycot_exceptions
I have explored the slycot exceptions mechanism, but for me, it's not sure that the SlycotResultWarning works correctly.

• fun(120) => Is this behavior right?
• fun([1,0]) => Is this behavior right?

The best way to make sure that the docsting parts Raises & Warns work would be to write pytests.

Should we refactor the pytests in this PR?
All the analysis pytests. If so, I would suggest that each wrapper function has its own test file:

• test_ab01nd.py
• test_ab05md.py
• ...

[KybernetikJo]

A rebase and squash of commits would also be good.

[bnavigator]

The info parameter depends on the Fortran routine, not all have it and not all define it the same way. Check the SLICOT docs.

The best way to make sure that the docsting parts Raises & Warns work would be to write pytests.

We have master/slycot/tests/test_analysis.py for that.

Which is separate from making sure that the wrapper function correctly raises the advertised exceptions. Those tests would need to be written, which would also increase code coverage.

[KybernetikJo]

In https://github.com/KybernetikJo/slycot-examples/tree/main/unstable/slycot_exceptions I have explored the slycot exceptions mechanism, but for me, it's not sure that the SlycotResultWarning works correctly.

• fun(120) => Is this behavior right?
• fun([1,0]) => Is this behavior right?

It was my mistake:
I changed fun.__doc__[:-60] to fun.__doc__[:-144] which is wrong, and I used assertRaises instead of assertWarns to catch a SlycotResultWarning.

Now the link is fine
slycot_exceptions.

We have master/slycot/tests/test_analysis.py for that.

I missed that one. So I will use this way to test the docstrings.

[bnavigator]

Suggested change

	If ``nr is None``, `tol`contains the tolerance for determining the
	If `nr` is `None`, `tol` contains the tolerance for determining the

[KybernetikJo]

It is true, there is a difference between monospace (not for variables) and single back-ticks (for variable, module, function) in the numpydoc style (https://numpydoc.readthedocs.io/en/latest/format.html#common-rest-concepts).

For paragraphs, indentation is significant and indicates indentation in the output. New paragraphs are marked with a blank line.

Use *italics*, **bold** and ``monospace`` if needed in any explanations (but not for variable names and doctest code or multi-line code). Variable, module, function, and class names should be written between single back-ticks (`numpy`).

A more extensive example of reST markup can be found in [this example document](http://docutils.sourceforge.net/docs/user/rst/demo.txt); the [quick reference](http://docutils.sourceforge.net/docs/user/rst/quickref.html) is useful while editing.

Line spacing and indentation are significant and should be carefully followed.

I think this kind of numpydoc correction should be done after the merge of the four basic clean-ups (math, transform, analysis, synthesis). The use of "double back-ticks" and "single back-ticks" is all over slycot.

I would prefer to fix the pytest file-structure first and the main numpydoc organization. After the merge of all 4 open PR's we can do smaller numpydoc fixes.

[bnavigator]

Okay, but please add the space after tol now.

[bnavigator]

I'm not a fan of these. They pollute the report about skipped tests but increase collection time for no real benefit

[KybernetikJo]

So this should be deleted again?
Then we have a wrapper, but no test.

Is that OK?

[bnavigator]

I'd rather have no test than a test that does nothing. We already have the missing coverage as indication that there should be one implemented

[KybernetikJo]

OK

[bnavigator]

Please keep the parametrization together. Splitting them out is unnecessary code repetition.

[bnavigator]

This is huge work. Thanks for still working on these. I'd like to do one or two more iterations before we merge, though.

[KybernetikJo]

@bnavigator Thanks for your comments. I will look into your feedback.

As I have said in #211 (comment), I am not sure about splitting the tests into single files. However, to have all tests in one file is also suboptimal.

We could organize the tests after the first 4 characters. The docstring-info tests (former test_analysis.py), however, in an extra file.
The files would then be organized as follows:

test_analysis_info.py (former test_analysis.py)
test_ab01.py
test_ab04.py
test_ab05.py
...
test_ab13.py
test_ag08.py

Or are there other options?
Any suggestions are welcome.

[KybernetikJo]

Another design decision concerns my use of test classes, even if there is only one test.

class Test_ab01nd:

    def test_ab01nd(self):

One reason for the use of classes was the splitting of the test_analysis.py file. #206 (comment)

class Test_ab09nd:

    @pytest.mark.parametrize(
    'fun,              exception_class,       erange,         checkvars',
    ((analysis.ab09nd, SlycotArithmeticError, 3,              {'alpha': -0.1}),
     (analysis.ab09nd, SlycotResultWarning,   ((1, 0), (2, 0)), {'nr': 3,
                                                               'Nr': 2,
                                                               'alpha': -0.1}),))
    def test_ab09nd_docparse(self, fun, exception_class, erange, checkvars):
        assert_docstring_parse(fun.__doc__,  exception_class, erange, checkvars)

Should we keep this?

[bnavigator]

Yes, please keep the docstring tests in one single file.

Please remove the classes. They're just duplicating the name for no additional functionality.

[KybernetikJo]

What do you think about the file structure? #206 (comment)

[bnavigator]

What do you think about the file structure? #206 (comment)

Splitting it out is okay as long as you don't duplicate code along with it. I'd rather keep it together as much as possible without losing the overview. So test_aXNN.py or even test_analysis_aX.py might indeed be a good compromise.

[KybernetikJo]

I also could just drop all changes to pytest files. We could do a reorganization in other pull requests, if necessary.

I think I have not added a single pytest to the project. I believe we would not lose anything.
It was only about reorganization, which was triggered by some misunderstanding of the docstring-pytest (info).
Actually, I was not aware about the whole test_analysis.py thing, when I started.

This would make the PR cleaner. It would be mainly about numpydoc changes, as the PR name suggests.

[bnavigator]

This would make the PR cleaner. It would be mainly about numpydoc changes, as the PR name suggests.

That's a very good point. Please feel free to discuss/propose structure changes in another PR.

[bnavigator]

LGTM. Ready to merge?

[bnavigator]

Thank you very much for the hard work and the patience.