Refactor/python docstrings for sd estimator

Dioprz commented 1 month ago

Description

Implementation of https://github.com/Crypto-TII/CryptographicEstimators/pull/169 for SDEstimator.

Formatting changes were made by Black.

Review process

A quick glance at the new docstrings would be great.

Estimator-specific doctests

make docker-build
make docker-run
pytest --doctest-modules -n auto -vv cryptographic_estimators/SDEstimator/

Cumulative test (with all the already migrated docstrings)

make docker-pytest

Pre-approval checklist

[x] The code builds clean without any errors or warnings
[x] I've added/updated tests
[x] I've added/updated documentation if needed

Dioprz commented 1 month ago

@Javierverbel Thank you so much for fixing the Sonarcloud warning 💯 .

Dioprz commented 1 month ago

only small comments, mainly about the citations and if the html doc generation still works

Thank you for the careful review, @FloydZ.

Some things that are worth explaining:

Sage uses reStructuredText-based docstrings, while Google-style docstrings don't (at least by default). This implies that:
- The current HTML docs generation process will likely be incompatible with the new docstring style out of the box. I believe we will need, for example, the Sphinx Napoleon extension to render our new docstring style.
- Because I wasn't expecting retrocompatibility, and the main goal right now is to have a pure-python executable library, I wasn't taking too much care to preserve the reST format.
- This is relatively easy to fix later. Due to the citation format, we only need a regex expression to look for places where the format [XXXX] is present and modify them as needed.
Regarding the more serious errors like removing citations or creating new reference sections with incorrect information, these were the result of a very experimental prompt and workflow process on my side. Because I concentrated so much on not creating or modifying values for the Examples: or Tests: sections, I completely forgot to refine the process for the other sections of the docstrings.
Once I sent this PR, I immediately improved the prompt used as well as my workflow process with the learnings made. Now I can inspect and review more carefully what is being generated by AI, so these errors shouldn't be present in subsequent PRs.

With that said, I left a 👍🏻 in every comment I applied, and a 👀 in those that requires feedback or consideration based on this explanation. Resolve those that you think are good now, and ping me to address any others that are missing please.

Dioprz commented 1 month ago

Alright. Digging more in the Napoleon extension for Sphinx, I found that:

It's a preparser able to translate from Google-styled docstrings to typical reST python docstrings.
For this reason, we still have all the power of reST where needed.
With that said, I strongly discourage the usage of reST elements where avoidable. Basically because it's usage requires to learn about how to express a mathematical expression, a table, or whatever else.
Napoleon provides a good number of sections and functionalities for most use-cases (Supported sections, docstring types and some configuration options). So let's just apply the KISS principle here.
I completely agree with making the exception for hyperlink references with the trailing _. I will apply the changes.
@FloydZ I was using backticks for monospaced font rendering. But looking at the Sphinx documentation on the topic, single backtick usage is context-dependant. What do you think we should do with that then? (EDIT: Precisely what I'm asking for is: what we should do with references to, for example: function arguments references, short math formulas, etc?)

Nice to hear your thoughts too: @Memphisd @Javierverbel

EDIT: I'm not able to generate the docs to make tests, because some MAYO-related errors arise

Dioprz commented 1 month ago

All the actionable comments were applied in 96c10fe. My last question in the previous comment shouldn't be a blocker in case we want to merge this now; but I'm also open to new comments.

Please don't review #178 and #179 until I have implemented the same changes as here. I will ping you all when those are done.