Clean documentation and add type hints #228

Daval-G · 2024-12-18T20:08:12Z

Just cleaning the documentation a bit and adding type hints

paquiteau · 2024-12-18T22:30:05Z

I am not against typing, however we should do it properly have you considered using mypy and /or ruff (the ANN* errors are disabled for now).

The trajectory module is a great starting point for its usage, and would likely complement the docstring to help people play with them.

A few comments:

try to avoid Any
str type are most of the time limited to named values , you can use Literal instead
*args and **kwargs should be typed with the type of the elements, not list
typing np array is not as good as it could be the best future prood solution is to use np.typing.NDArray

Daval-G · 2024-12-19T22:33:12Z

Good point. I followed ruff's ANN rule, and the current code is compliant (except for bad usage of Literal apparently)

try to avoid Any

Done, but some cases like open kwargs are still relevant. I tried to reduce them to a minimum though.

str type are most of the time limited to named values , you can use Literal instead

Make sense. The CI tells me I underestimated Literal, I will fix that soon.

*args and **kwargs should be typed with the type of the elements, not list

Make sense.

typing np array is not as good as it could be the best future prood solution is to use np.typing.NDArray

Done, but I have to check also how it renders in docstrings

paquiteau

We are on a good path, thanks for the tedious work.

Do not hesitate to check your modification with mypy locally. I am also pretty sure an LLM could help you adding annotation in bulk if needed...

paquiteau · 2024-12-20T08:39:45Z

src/mrinufft/trajectories/display.py

        """Restore the display configuration."""
        for key, value in self._old_values.items():
            setattr(displayConfig, key, value)
        delattr(self, "_old_values")

-    def __enter__(self):
+    def __enter__(self) -> "displayConfig":


if you use from __future__ import annotations you should be able to annotate with the Class directly, not a string :)

Delayed annotation checks helped in several cases actually, thx !

paquiteau · 2024-12-20T08:42:06Z

src/mrinufft/trajectories/display.py

-    constraints_order=None,
-    **constraints_kwargs,
-):
+    trajectory: np.typing.NDArray,


I think you can just from numpy.typing import NDArray and use it everywhere

Done, much better indeed

paquiteau · 2024-12-20T08:43:59Z

src/mrinufft/trajectories/inits/random_walk.py

+    *,
+    diagonals: bool = True,
+    pseudo_random: bool = True,
+    **sampling_kwargs: Literal | bool,


Literal has to be used as Literal["A string"] or Literal[1] for instance

Done. Most Literal uses were actually unjustified because of the way we handle Enum for everything

paquiteau · 2024-12-20T08:46:15Z

src/mrinufft/trajectories/inits/random_walk.py

+    diagonals: bool = True,
+    pseudo_random: bool = True,
+    **sampling_kwargs: Literal | bool,
+) -> np.ndarray:


should be NDArray

Yes I missed a lot of them for some reason, thx

Daval-G · 2024-12-25T21:22:38Z

mypy is still showing some issues but I reduced them from 120 to 25, with half of them outside the trajectory submodule for some reason. The current code is compliant with ruff's ANN rules

paquiteau · 2024-12-27T09:27:04Z

src/mrinufft/trajectories/inits/travelling_salesman.py

+    first_cluster_by: Literal["x", "y", "z", "r", "phi", "theta"] | None = None,
+    second_cluster_by: Literal["x", "y", "z", "r", "phi", "theta"] | None = None,
+    sort_by: Literal["x", "y", "z", "r", "phi", "theta"] | None = None,


I think you can:

Use a type alias (declared below the imports), for brievety

Put the None inside the literal

Done for the type alias, thx

There is one function where None is not an acceptable option so I kept it out

Clean documentation and add type hints

d526791

Daval-G added the documentation Improvements or additions to documentation label Dec 18, 2024

Daval-G requested a review from paquiteau December 18, 2024 20:08

Daval-G self-assigned this Dec 18, 2024

Daval-G added 3 commits December 19, 2024 23:15

Apply ruff ANN rules to trajectories

a0031a3

Replace np.ndarray type hints with np.typing.NDArray

d5b3e09

Fix missing import

e4314ac

paquiteau requested changes Dec 20, 2024

View reviewed changes

Fix Literal use, change types to NDArray and reduce mypy errors

914fc3f

paquiteau reviewed Dec 27, 2024

View reviewed changes

Add type alias for clustering coordinates

ff493e9

Daval-G requested a review from paquiteau December 27, 2024 16:25

paquiteau approved these changes Jan 10, 2025

View reviewed changes

paquiteau merged commit 3abd8d9 into master Jan 10, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean documentation and add type hints #228

Clean documentation and add type hints #228

Daval-G commented Dec 18, 2024

paquiteau commented Dec 18, 2024 •

edited

Loading

Daval-G commented Dec 19, 2024

paquiteau left a comment

paquiteau Dec 20, 2024

Daval-G Dec 25, 2024

paquiteau Dec 20, 2024

Daval-G Dec 25, 2024 •

edited

Loading

paquiteau Dec 20, 2024

Daval-G Dec 25, 2024

paquiteau Dec 20, 2024

Daval-G Dec 25, 2024

Daval-G commented Dec 25, 2024 •

edited

Loading

paquiteau Dec 27, 2024

Daval-G Dec 27, 2024

Clean documentation and add type hints #228

Clean documentation and add type hints #228

Conversation

Daval-G commented Dec 18, 2024

paquiteau commented Dec 18, 2024 • edited Loading

Daval-G commented Dec 19, 2024

paquiteau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daval-G Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daval-G commented Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paquiteau commented Dec 18, 2024 •

edited

Loading

Daval-G Dec 25, 2024 •

edited

Loading

Daval-G commented Dec 25, 2024 •

edited

Loading