Fix some type hinting to help with migrating `Distribution` #7484

thomasaarholt · 2024-08-29T10:08:37Z

Description

This is a small PR. I've added and fixed type hinting to a few lines of code. The idea is to make it easier to migrate the Distribution class to a function-based approach as discussed in the related issue.

Related Issue

Related to Add distribution parameters as named positional arguments #6083

Checklist

Checked that the pre-commit linting/style checks pass
Included tests that prove the fix is effective or that the new feature works
Added necessary documentation (docstrings and/or example notebooks)
If you are a pro: each commit corresponds to a relevant logical change

Type of change

📚 Documentation preview 📚: https://pymc--7484.org.readthedocs.build/en/7484/

codecov · 2024-08-29T10:42:11Z

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.16%. Comparing base (d596afb) to head (157c29a).

Files with missing lines	Patch %	Lines
pymc/distributions/distribution.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7484      +/-   ##
==========================================
- Coverage   92.16%   92.16%   -0.01%     
==========================================
  Files         103      103              
  Lines       17200    17203       +3     
==========================================
+ Hits        15853    15855       +2     
- Misses       1347     1348       +1

Files with missing lines	Coverage Δ
pymc/distributions/continuous.py	`97.92% <100.00%> (+<0.01%)`	⬆️
pymc/distributions/distribution.py	`91.66% <80.00%> (-0.23%)`	⬇️

ricardoV94 · 2024-08-31T12:58:58Z

mypy is not happy with the changes? https://github.com/pymc-devs/pymc/actions/runs/10613016885/job/29415940076?pr=7484

thomasaarholt · 2024-09-02T07:19:49Z

Trying a solution, see below.

Yes. That's this commit, where I fixed the incorrect type hint syntax.

All three reported errors are the same:

Incompatible types in assignment 
(expression has type "Callable[[Any, VarArg(Any), DefaultNamedArg(Any, 'size')], Any]", 
base class "Distribution" defined the type as "RandomVariable | SymbolicRandomVariable")

and are due to Mixture, Censored and Discrete distributions having rv_op = <a method>, while e.g. Normal has rv_op = <a class>.

class Censored(Distribution):
    rv_type = CensoredRV
    rv_op = CensoredRV.rv_op # <-- a method

class Normal(Distribution):
    rv_op = NormalRV() # <-- an instance of NormalRV, which is a subclass of RandomVariable

Solution
Typing the above, I realize that since we are really wanting to type this by behaviour rather than state. So I've tried typing it as Callable[..., TensorVariable] instead, which is how it always behaves (I think).

ricardoV94 · 2024-09-02T08:31:14Z

sounds correct

thomasaarholt · 2024-09-02T11:21:25Z

I am not going to worry about the error in func_utils.py. It's in find_constrained_prior, which is a) deprecated and b) not typed correctly (first argument should be type[Distribution]).

I am currently testing if I can forego the = None on the rv_op and rv_type completely. I don't think these are necessary.

thomasaarholt · 2024-09-02T13:23:28Z

The failing test is unrelated I guess? @ricardoV94

I think I'm finished with this one 😊

ricardoV94 · 2024-09-02T13:52:49Z

I am not going to worry about the error in func_utils.py. It's in find_constrained_prior, which is a) deprecated and b) not typed correctly (first argument should be type[Distribution]).

I am currently testing if I can forego the = None on the rv_op and rv_type completely. I don't think these are necessary.

Then can you remove the type hints, we can't merge with a known failing CI or it will be a source of confusion for everyone else

thomasaarholt · 2024-09-02T16:39:07Z

I can remove the type hint for class attribute rv_op, but then I either need to leave it = None (which the passing tests just established is unnecessary), replace it (with Any, probably, and a comment explaining what I've learnt about it), or remove the entire attribute, since the following is not legal Python:

class Foo:
    bar

Use of bound_arg_indices shows that this should be a (start, end) tuple, not a list.

I don't want to touch the `= None` part. I could add ` | None` to the type hint but I suspect the `= None` is unnecessary.

This is a bit like using a Protocol class, we say that whatever is passed to rv_op, it should return a TensorVariable.

I don't think these are necessary at all. The tests will tell us so.

thomasaarholt · 2024-10-04T10:08:41Z

@ricardoV94 mind taking a look?

ricardoV94 · 2024-10-07T23:39:45Z

pymc/distributions/distribution.py

+            if class_change_dist_size is not None:
+                raise ValueError("HAHAHA")


Fun debug statement left :)

Suggested change

if class_change_dist_size is not None:

raise ValueError("HAHAHA")

Hehe! That's what I get for not adding my changes line by line for this particular commit!

ricardoV94 · 2024-10-07T23:41:12Z

pymc/distributions/continuous.py

@@ -152,7 +152,7 @@ class BoundedContinuous(Continuous):
    """Base class for bounded continuous distributions"""

    # Indices of the arguments that define the lower and upper bounds of the distribution
-    bound_args_indices: list[int] | None = None
+    bound_args_indices: tuple[int, int] | None = None


Aren't there distributions with only one of the bounds (lower, upper)?

You're right, there are! I was wondering why mypy didn't flag it, but of course it was originally an error as list[int] doesn't allow (3, None)! Fixing this

Allows optional lower or upper bounds.

thomasaarholt · 2024-10-08T11:56:17Z

The failing tests stem from this commit, not this PR.

ricardoV94 · 2024-10-08T12:40:55Z

@thomasaarholt should be fixed by #7529

ricardoV94 · 2024-10-09T07:42:17Z

Thanks @thomasaarholt

ricardoV94 approved these changes Aug 31, 2024

View reviewed changes

ricardoV94 added maintenance no releasenotes Skipped in automatic release notes generation labels Aug 31, 2024

thomasaarholt added 6 commits September 4, 2024 15:01

Type get_tau_sigma correctly and fix comment

85784b8

Fix tuple type

8ea8b6c

Use of bound_arg_indices shows that this should be a (start, end) tuple, not a list.

Fix incorrect type hinting syntax

b2692ea

I don't want to touch the `= None` part. I could add ` | None` to the type hint but I suspect the `= None` is unnecessary.

Type rv_op based on behaviour rather than type

af91def

This is a bit like using a Protocol class, we say that whatever is passed to rv_op, it should return a TensorVariable.

Try removing Optional / = None completely

2e84b30

I don't think these are necessary at all. The tests will tell us so.

Try setting rv_op: Any = None

157c29a

thomasaarholt force-pushed the typing-some branch from 334b232 to 157c29a Compare September 5, 2024 05:49

ricardoV94 requested changes Oct 7, 2024

View reviewed changes

ricardoV94 reviewed Oct 7, 2024

View reviewed changes

thomasaarholt added 2 commits October 8, 2024 09:24

Remove debug statement

7bd35a8

Correct type definition for bound_arg_indices

1575243

Allows optional lower or upper bounds.

ricardoV94 approved these changes Oct 8, 2024

View reviewed changes

twiecki approved these changes Oct 8, 2024

View reviewed changes

ricardoV94 merged commit 938aff4 into pymc-devs:main Oct 9, 2024
17 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some type hinting to help with migrating `Distribution` #7484

Fix some type hinting to help with migrating `Distribution` #7484

thomasaarholt commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

ricardoV94 commented Aug 31, 2024

thomasaarholt commented Sep 2, 2024

ricardoV94 commented Sep 2, 2024

thomasaarholt commented Sep 2, 2024

thomasaarholt commented Sep 2, 2024

ricardoV94 commented Sep 2, 2024

thomasaarholt commented Sep 2, 2024

thomasaarholt commented Oct 4, 2024

ricardoV94 Oct 7, 2024

thomasaarholt Oct 8, 2024

ricardoV94 Oct 7, 2024 •

edited

Loading

thomasaarholt Oct 8, 2024

thomasaarholt commented Oct 8, 2024

ricardoV94 commented Oct 8, 2024

ricardoV94 commented Oct 9, 2024

		if class_change_dist_size is not None:
		raise ValueError("HAHAHA")

Fix some type hinting to help with migrating Distribution #7484

Fix some type hinting to help with migrating Distribution #7484

Conversation

thomasaarholt commented Aug 29, 2024 • edited Loading

Description

Related Issue

Checklist

Type of change

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

ricardoV94 commented Aug 31, 2024

thomasaarholt commented Sep 2, 2024

ricardoV94 commented Sep 2, 2024

thomasaarholt commented Sep 2, 2024

thomasaarholt commented Sep 2, 2024

ricardoV94 commented Sep 2, 2024

thomasaarholt commented Sep 2, 2024

thomasaarholt commented Oct 4, 2024

ricardoV94 Oct 7, 2024

Choose a reason for hiding this comment

thomasaarholt Oct 8, 2024

Choose a reason for hiding this comment

ricardoV94 Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

thomasaarholt Oct 8, 2024

Choose a reason for hiding this comment

thomasaarholt commented Oct 8, 2024

ricardoV94 commented Oct 8, 2024

ricardoV94 commented Oct 9, 2024

Fix some type hinting to help with migrating `Distribution` #7484

Fix some type hinting to help with migrating `Distribution` #7484

thomasaarholt commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

ricardoV94 Oct 7, 2024 •

edited

Loading