Improve typing and docstrings #87

felixblanke · 2024-06-04T00:46:34Z

This addresses #85 and changes type hints throughout the code base.

Notably, I added two type aliases as return types for wavelet transforms:

WaveletCoeffNd which corresponds to (Tensor, *(WaveletDetailDict, ...))
WaveletCoeff2d which corresponds to (Tensor, *(WaveletDetailTuple2d, ...))

which make use of the following two new type aliases for detail coefficients:

WaveletDetailDict which corresponds to dict[str, Tensor]
WaveletDetailTuple2d which corresponds to (Tensor, Tensor, Tensor).

I also added the type alias WaveletCoeff2dSeparable which is an alias for WaveletCoeffNd to emphasize the different return type in case of the 2d separable transform

(I would be very much open to suggestions how to name those differently)

Importantly: The collection that contains the wavelet coefficients changes from list to tuple. Therefore, this changes the return types of our wavelet coefficients, not "only" for type hinting but also with possible impact on user code (e.g. we change the collection from mutable to immutable).

Further changes:

Some typing is narrowed (e.g. by replacing list by sequence)
For typing we switch from typing.List to the builtin list (and the same for dict and tuple). This was introduced in python 3.9 (our minimum version)
I adopted the recommended flake8 config by the black project to address differing opinions between both projects.
Minor improvements to the docs and docstrings

Important note for checking the type hints:
The overloads for _map_result introduced in the _util module currently lead to a crash in mypy (relevant issue). Thus, running the nox typing session will fail.

Update: A fix has been merged to mypy's master branch (relevant PR). To install from the git repository, replace in setup.cfg:70 the line mypy by

    mypy @ git+https://github.com/python/mypy

Renamed * `WaveletTransformReturn3d` to `WaveletCoeffDetailDict`, * `WaveletTransformReturn2d` to `WaveletCoeffDetailTuple2d` and added the type aliases * `WaveletDetailDict` for `dict[str, Tensor]` * `WaveletDetailTuple2d` for `tuple[Tensor, Tensor, Tensor]`

v0lta · 2024-06-06T14:56:59Z

Thanks a lot, @felixblanke. Since it's older, I will start with #84 and then move on to this PR.

cthoyt · 2024-06-07T10:28:14Z

src/ptwt/_stationary_transform.py

@@ -28,7 +29,7 @@ def _swt(
        level (Optional[int], optional): The number of levels to compute

    Returns:
-        List[torch.Tensor]: Same as wavedec.
+        list[torch.Tensor]: Same as wavedec.


as you go through these, it's better to remove the redundant type annotations inside the docstrings rather than update them

Suggested change

list[torch.Tensor]: Same as wavedec.

: Same as wavedec.

Should we keep the leading colons? The examples in Google's styleguide show none.

if the documentation works without it, then no! I am not usually using this code style so I don't know all the specifics

I went through the docstrings and cleaned them up. This should be addressed now.

The styleguide also writes about function arguments: "The description should include required type(s) if the code does not contain a corresponding type annotation." Opinions on this matter?

cthoyt · 2024-06-07T10:28:56Z

src/ptwt/_util.py

@@ -29,6 +31,15 @@ def __len__(self) -> int:
        return len(self.dec_lo)


+WaveletDetailTuple2d = tuple[torch.Tensor, torch.Tensor, torch.Tensor]


it's useful to add a docstring to each of these tuples to give some additional context about what they are and where they're used

also, this could be a place to use a namedtuple to make it even more explicit

Thats a good idea 👍

src/ptwt/_util.py

src/ptwt/conv_transform_2.py

Co-authored-by: Charles Tapley Hoyt <[email protected]>

felixblanke · 2024-06-11T16:00:29Z

I just noticed that that the type aliases like WaveletCoeffDetailTuple2d are not properly displayed in the docs when I build them. I will look into that tomorrow

src/ptwt/_stationary_transform.py

Fixates some type aliases such that the API documentation shows the type alias name instead of its resolved value. For this feature to work we need to import the 'annotations' feature from __future__

felixblanke · 2024-06-14T01:15:14Z

I moved the type aliases into the constants module to get them into the public namespace and to let them show up in the documentation. I also changed the docs behavior to not resolve these type aliases. To do this we need to import

from __future__ import annotations

in all modules using the type aliases.

Further, as suggested by @cthoyt I changed WaveletDetailTuple2d from tuple[Tensor, Tensor, Tensor] into a named tuple. This should not change user code behavior. However, people might now see type checker warnings that they are passing tuples instead of namedtuples into the inverse transformations. Any ideas how to mitigate this?

Rename * `WaveletCoeffDetailDict` to `WaveletCoeffNd` * `WaveletCoeffDetailTuple2d` to `WaveletCoeff2d`

felixblanke · 2024-06-14T12:34:40Z

This PR changes the returned collection for higher dimensional wavelet transforms from list to tuple to enable better type hints. This is not done for the 1d case where we still return list[Tensor].
Should we change this to tuple[Tensor, ...] to stay consistent with the higher dimensional transforms? Or should we just leave it as is? @v0lta

v0lta · 2024-06-14T12:42:09Z

https://pywavelets.readthedocs.io/en/latest/ref/dwt-discrete-wavelet-transform.html#multilevel-decomposition-using-wavedec also returns a list, although consistency would, of course, be nice. While I lean towards leaving things as is, I would accept the change for consistency. However, if someone comes from pywt and has code that builds in the list type, this could cause trouble. On the other hand, that's unlikely to happen. Hence, I am just leaning toward letting it be how it is.

Make the functions creating the natural and frequency packet order for WaveletPacket2d static methods of WaveletPacket2d. This changes * `get_natural_order` from instance to static function * `get_frequency_order` from separate func to static function in the scope of WaveletPacket2d Further, to make both methods consistent, `get_frequency_order` now returns concatenated strings instead of a tuple of single char strings.

felixblanke · 2024-06-14T14:58:47Z

I also went through the examples. In consequence, I refactored the functions creating the natural and frequency packet order for WaveletPacket2D.
I changed get_natural_order and get_frequency_order both to be static functions of WaveletPacket2D.
Further, to make both methods consistent, get_frequency_order now returns concatenated strings instead of a tuple of single char strings.

v0lta · 2024-06-18T10:33:31Z

do we add mypy @ git+https://github.com/python/mypy to setup.py until the next mypy release?

v0lta · 2024-06-18T20:07:21Z

okay. I think we are done here. Thanks a lot @felixblanke and @cthoyt . I will merge this and close #85 . Feel free to reopen if we are still missing something.

felixblanke added 17 commits June 3, 2024 12:29

Exchange List with Sequence in args

8ff2c89

Use builtin list instead of List

aacca4b

Use builtin tuple instead of Tuple

fa14797

Use builtin dict instead of Dict

75c0bc0

Change return types to tuple

69c69a2

Add function overloads

3126c89

refactor _map_result

46198d0

More adaptions to the new types

168b4a0

Fix _map_result

09811b0

Improve wavelet args for separable transforms

cb1b3db

tighten return type

c2dccd0

Format

57c1ab7

Address linter remarks

9950294

Adopt flake8 rules recommended by black project

c9af10c

Remove matplotlib requirement

df1884e

Minor typing improvement

414492d

v0lta self-assigned this Jun 6, 2024

cthoyt reviewed Jun 7, 2024

View reviewed changes

src/ptwt/_util.py Outdated Show resolved Hide resolved

cthoyt reviewed Jun 7, 2024

View reviewed changes

src/ptwt/conv_transform_2.py Outdated Show resolved Hide resolved

cthoyt and others added 4 commits June 10, 2024 16:16

Merge branch 'main' into improve-typing

da7ac93

Change error type to ValueError at input validation

f495acc

Co-authored-by: Charles Tapley Hoyt <[email protected]>

Improve type hints

acd8769

Fix axis typehint and add to docstr

6d60132

v0lta added this to the v0.1.9 milestone Jun 12, 2024

v0lta reviewed Jun 13, 2024

View reviewed changes

src/ptwt/_stationary_transform.py Outdated Show resolved Hide resolved

felixblanke added 10 commits June 14, 2024 00:53

Fixate some type aliases to not be resolved in docs.

5051f13

Fixates some type aliases such that the API documentation shows the type alias name instead of its resolved value. For this feature to work we need to import the 'annotations' feature from __future__

Improve docstrings

1c6b4ea

Replace redundant docstr info with refers to type alias

23ed9d8

Add some special member funcs to docs

1139876

Fix favicon and move version module in API

dd10c82

Fix import

0f3d01a

Change detail coeff tuple to namedtuple

cd4c131

Change type str back to imperative mood

b1c34d5

Merge branch 'main' into improve-typing

3dcecbe

Add comment clarification

d4ed327

felixblanke changed the title ~~Improve typing~~ Improve typing and docstrings Jun 14, 2024

felixblanke added 4 commits June 14, 2024 13:16

Rename type aliases.

1991a07

Rename * `WaveletCoeffDetailDict` to `WaveletCoeffNd` * `WaveletCoeffDetailTuple2d` to `WaveletCoeff2d`

Introduce WaveletCoeff2dSeparable alias

3939070

Make cast in _map_result more narrow and fix tuple creation

b324728

Update typing in tests

a55f223

felixblanke added 5 commits June 14, 2024 14:46

Also import 2d separable type in __init__

48e87a7

Improve type alias docstr

4282c9d

Improve typing in JIT code

942ad60

Adapt right pad logic to avoid JIT tracer warning

b8a510a

v0lta added the invalid This doesn't seem right label Jun 17, 2024

Use mypy main branch for typing session for now

c973350

v0lta merged commit c256d9e into main Jun 18, 2024
7 checks passed

cthoyt deleted the improve-typing branch June 18, 2024 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve typing and docstrings #87

Improve typing and docstrings #87

felixblanke commented Jun 4, 2024 •

edited

Loading

v0lta commented Jun 6, 2024

cthoyt Jun 7, 2024

felixblanke Jun 11, 2024

cthoyt Jun 11, 2024

felixblanke Jun 11, 2024

cthoyt Jun 7, 2024

cthoyt Jun 11, 2024

felixblanke Jun 11, 2024

felixblanke commented Jun 11, 2024

felixblanke commented Jun 14, 2024

felixblanke commented Jun 14, 2024

v0lta commented Jun 14, 2024

felixblanke commented Jun 14, 2024

v0lta commented Jun 18, 2024

v0lta commented Jun 18, 2024

		@@ -29,6 +31,15 @@ def __len__(self) -> int:
		return len(self.dec_lo)


		WaveletDetailTuple2d = tuple[torch.Tensor, torch.Tensor, torch.Tensor]

Improve typing and docstrings #87

Improve typing and docstrings #87

Conversation

felixblanke commented Jun 4, 2024 • edited Loading

v0lta commented Jun 6, 2024

cthoyt Jun 7, 2024

Choose a reason for hiding this comment

felixblanke Jun 11, 2024

Choose a reason for hiding this comment

cthoyt Jun 11, 2024

Choose a reason for hiding this comment

felixblanke Jun 11, 2024

Choose a reason for hiding this comment

cthoyt Jun 7, 2024

Choose a reason for hiding this comment

cthoyt Jun 11, 2024

Choose a reason for hiding this comment

felixblanke Jun 11, 2024

Choose a reason for hiding this comment

felixblanke commented Jun 11, 2024

felixblanke commented Jun 14, 2024

felixblanke commented Jun 14, 2024

v0lta commented Jun 14, 2024

felixblanke commented Jun 14, 2024

v0lta commented Jun 18, 2024

v0lta commented Jun 18, 2024

felixblanke commented Jun 4, 2024 •

edited

Loading