Fix convert_size failure with 1d symbolic vectors #5417

5hv5hvnk · 2022-01-28T13:07:40Z

convert_size wrongly assumes symbolic sizes have to be scalars

import pymc as pm
import aesara.tensor as at

s = at.scalar('s')
size = at.stack([s, s])
x = at.random.normal(size=size)
x.eval({s: 2})
# array([[-0.66285345,  0.97341598],
#        [ 0.05158132,  2.03399059]])

pm.Normal.dist(size=size)
# ValueError: The `size` parameter must be a tuple, TensorVariable, int or list. Actual: <class 'aesara.tensor.var.TensorVariable'>

fixed by changing code in

pymc/pymc/distributions/shape_utils.py

Line 475 in 9155922

    
           if isinstance(size, int) or (isinstance(size, TensorVariable) and size.ndim == 0):

by accepting size.ndim == 1 as well as size.ndim == 0

Closes #5394

michaelosthege

Please add unit tests that can detect the original issue, ideally testing the function with all different kinds of inputs that should be supported according to its signature.
Also you should edit the description of the PR to explain what this is about: what was the problem with the original code and how do your changes fix it?

ricardoV94

Thanks for opening the PR. I've left a comment above

pymc/distributions/shape_utils.py

5hv5hvnk · 2022-01-31T00:16:19Z

@ricardoV94 I made a new edit making sure it only accepts ndim=0 and ndim=1 please have a look

ricardoV94 · 2022-02-01T10:57:59Z

@5hv5hvnk We need tests showing that this is no longer an issue

5hv5hvnk · 2022-02-04T23:46:30Z

@ricardoV94 how do I make test cases and show that they are working? As of now when I tested few different things on the code that was in the initial issue itself and they work fine.

ricardoV94 · 2022-02-15T14:42:41Z

Hi @5hv5hvnk, apologies for the delay.

You can add a new test case in this function:

pymc/pymc/tests/test_shape_handling.py

Lines 463 to 468 in 9155922

    
           def test_convert_size(self): 
        
               assert convert_size(7) == (7,) 
        
               with pytest.raises(ValueError, match="tuple, TensorVariable, int or list"): 
        
                   convert_size(size="notasize") 
        
               with pytest.raises(ValueError, match="cannot contain"): 
        
                   convert_size(size=(3, ...))

Something like this might be enough:

x = at.constant(5)
assert convert_size(x) is size

x = at.constant([5, 5])
assert convert_size(x) is size

ricardoV94 · 2022-02-15T14:47:44Z

pymc/distributions/shape_utils.py

+    if isinstance(size, int) or (isinstance(size, TensorVariable) and size.ndim==1 or size.ndim==0):
+        size = tuple(size,)
    elif isinstance(size, (list, tuple)):
        size = tuple(size)
    else:


Does this work? I don't think we have to do anything if it is already a TensorVariable. We could assert it has ndim < 2. but that's about it.

Suggested change

if isinstance(size, int) or (isinstance(size, TensorVariable) and size.ndim==1 or size.ndim==0):

size = tuple(size,)

elif isinstance(size, (list, tuple)):

size = tuple(size)

else:

if isinstance(size, int):

size = tuple(size,)

elif isinstance(size, (list, tuple)):

size = tuple(size)

elif not isinstance(TensorVariable):

ricardoV94 · 2022-02-15T15:49:56Z

@5hv5hvnk This error also seems to be present in convert_shape:

pymc/pymc/distributions/shape_utils.py

Lines 475 to 476 in 90948a3

    
           if isinstance(size, int) or (isinstance(size, TensorVariable) and size.ndim==1 or size.ndim==0): 
        
               size = tuple(size,)

A similar fix should work there

michaelosthege

Please run the pre-commit locally before pushing l:
pip install pre-commit

pre-commit run --all

michaelosthege · 2022-03-12T09:22:48Z

pymc/distributions/shape_utils.py

-        size = (size,)
+    if isinstance(size, int):
+        size = size
+    elif isinstance(size,TensorVariable):


Please check for the ndim too (like a few lines above). For ndim>1 we still want to run into the exception case.

michaelosthege · 2022-03-12T14:32:48Z

pymc/distributions/shape_utils.py

+    elif isinstance(size, TensorVariable) and size.ndim <= 1:
+        size = tuple(size)


tuple(size) does't work when size.ndim == 0.
The 0 and 1 ndim must be separate cases

does to_tuple work?

something like this would be sufficient ?

if isinstance(shape, int) or (isinstance(size,TensorVariable) and shape.ndim==0): shape = shape if isinstance(shape, TensorVariable) and shape.ndim == 1: shape = tuple(shape)

Just keep the code that was there before and add another elif case:

if isinstance(size, int) or (isinstance(size, TensorVariable) and size.ndim == 0): size = (size,) elif isinstance(size, TensorVariable) and size.ndim == 1: size = tuple(size)

And the same further up in convert_shape

on using

pre-commit run --all

all the tests are passed, however it shows build failed when I commit the changes how can I resolve this?

Precommit is not running tests, but other things like linters and code format checks. The output should tell you what the fix is, or automatically add the changes to git staging. Feel free to post a screenshot of the output and we can more specifically let you know what to do!

this shows pre commit works fine, yet the commit fails to build. How can I fix this?

Im not sure what you mean by build. This indicates the commit should have taken place. What happens when you try and push it to github?

I think your branch could be outdated. The pre-commit in the CI failed in the type checking step but that step did not run in your screenshot.

Do the following to update your branch:

Make sure that you don't have uncommitted changes (commit, stash or revert them if needed)

git checkout main

git pull

git checkout my-feature to go back to the branch of this PR

git rebase main to rebase the PR branch onto main. Since GitHub doesn't show any merge conflicts this step should work without problems.

Then run pre-commit again. This time it should reproduce the failure from the CI.

canyon289 · 2022-03-13T14:41:08Z

pymc/distributions/shape_utils.py

@@ -449,9 +449,10 @@ def convert_shape(shape: Shape) -> Optional[WeakShape]:
    """Process a user-provided shape variable into None or a valid shape object."""
    if shape is None:
        return None
-
-    if isinstance(shape, int) or (isinstance(shape, TensorVariable) and shape.ndim == 0):
+    if isinstance(shape, int) or (isinstance(size, TensorVariable) and shape.ndim == 0):


I would add parentheses around the or statement conditionsm its not strictly needed but it makes the code intent much more obvious

I don't mind the parantheses, but in the 2nd isinstance you changed shape to size.
This line should have no change at all

michaelosthege

I canceled the CI workflows because the pre-commit was failing.
Please run pre-commit before pushing to reduce the CI workload.

michaelosthege · 2022-03-13T14:48:40Z

pymc/distributions/shape_utils.py

@@ -449,9 +449,10 @@ def convert_shape(shape: Shape) -> Optional[WeakShape]:
    """Process a user-provided shape variable into None or a valid shape object."""
    if shape is None:
        return None
-
-    if isinstance(shape, int) or (isinstance(shape, TensorVariable) and shape.ndim == 0):
+    if isinstance(shape, int) or (isinstance(size, TensorVariable) and shape.ndim == 0):


I don't mind the parantheses, but in the 2nd isinstance you changed shape to size.
This line should have no change at all

michaelosthege · 2022-03-13T14:50:25Z

pymc/distributions/shape_utils.py

        shape = (shape,)
+    if isinstance(shape, TensorVariable) and shape.ndim == 1:


This should be an elif

canyon289 · 2022-03-13T15:24:23Z

I canceled the CI workflows because the pre-commit was failing. Please run pre-commit before pushing to reduce the CI workload.

Noted, and thank you

michaelosthege · 2022-03-14T08:25:46Z

The diff looks good.
I triggered the CI tests, but I'm afraid they might fail because the branch is outdated (similar to why the docs build failed).

codecov · 2022-03-14T08:36:43Z

Codecov Report

Merging #5417 (18e363a) into main (0dca647) will decrease coverage by 18.30%.
The diff coverage is 66.66%.

❗ Current head 18e363a differs from pull request most recent head f30c6b4. Consider uploading reports for the commit f30c6b4 to get more accurate results

@@             Coverage Diff             @@
##             main    #5417       +/-   ##
===========================================
- Coverage   81.39%   63.08%   -18.31%     
===========================================
  Files          82       76        -6     
  Lines       14213    13779      -434     
===========================================
- Hits        11568     8693     -2875     
- Misses       2645     5086     +2441

Impacted Files	Coverage Δ
pymc/distributions/shape_utils.py	`48.53% <66.66%> (-50.34%)`	⬇️
pymc/sampling_jax.py	`0.00% <0.00%> (-97.46%)`	⬇️
pymc/ode/utils.py	`15.62% <0.00%> (-84.38%)`	⬇️
pymc/smc/smc.py	`18.54% <0.00%> (-79.36%)`	⬇️
pymc/model_graph.py	`15.86% <0.00%> (-69.66%)`	⬇️
pymc/smc/sample_smc.py	`16.77% <0.00%> (-66.45%)`	⬇️
pymc/variational/updates.py	`21.67% <0.00%> (-66.01%)`	⬇️
pymc/func_utils.py	`22.50% <0.00%> (-65.00%)`	⬇️
pymc/ode/ode.py	`24.24% <0.00%> (-60.61%)`	⬇️
pymc/printing.py	`26.73% <0.00%> (-59.13%)`	⬇️
... and 50 more

5hv5hvnk · 2022-03-14T09:01:06Z

The diff looks good.
I triggered the CI tests, but I'm afraid they might fail because the branch is outdated (similar to why the docs build failed).
Do I have to clone the repo again and do changes there in order to update the branch or is there something else I can do?

twiecki · 2022-03-14T09:07:54Z

@5hv5hvnk You need to pull main and then rebase your branch and force-push: https://stackoverflow.com/questions/8965781/update-an-outdated-branch-against-master-in-a-git-repo

5hv5hvnk · 2022-03-14T10:37:09Z

@5hv5hvnk You need to pull main and then rebase your branch and force-push: https://stackoverflow.com/questions/8965781/update-an-outdated-branch-against-master-in-a-git-repo

This shows my branch is up to the date I still used push --force to push it to the main branch

twiecki · 2022-03-14T10:39:54Z

@5hv5hvnk This is your fork, you can add the pymc-devs repo as a remote and then pull to main from there and then rebase.

fix on issue 5394

9dadd97

5hv5hvnk mentioned this pull request Jan 28, 2022

convert_size wrongly assumes symbolic sizes have to be scalars #5394

Closed

michaelosthege requested changes Jan 29, 2022

View reviewed changes

ricardoV94 requested changes Jan 29, 2022

View reviewed changes

pymc/distributions/shape_utils.py Outdated Show resolved Hide resolved

new edit

90948a3

5hv5hvnk requested review from michaelosthege and ricardoV94 February 1, 2022 10:45

ricardoV94 requested changes Feb 15, 2022

View reviewed changes

ricardoV94 changed the title ~~fix on issue #5394~~ Fix convert_size failure with 1d symbolic vectors Feb 15, 2022

5hv5hvnk added 2 commits March 12, 2022 10:19

changed conver_size

72accb7

changed conver_size

2a6862a

michaelosthege requested changes Mar 12, 2022

View reviewed changes

added ndim<=1 condition

8a2851c

michaelosthege reviewed Mar 12, 2022

View reviewed changes

updated size.ndim==0 condition

c195048

canyon289 reviewed Mar 13, 2022

View reviewed changes

michaelosthege requested changes Mar 13, 2022

View reviewed changes

added elif

18e363a

Update shape_utils.py

f30c6b4

5hv5hvnk mentioned this pull request Mar 16, 2022

Updated convert_shape and convert_size #5601

Merged

This pull request was closed.

		elif isinstance(size, TensorVariable) and size.ndim <= 1:
		size = tuple(size)

		shape = (shape,)
		if isinstance(shape, TensorVariable) and shape.ndim == 1:

Uh oh!

Fix convert_size failure with 1d symbolic vectors #5417

Fix convert_size failure with 1d symbolic vectors #5417

Uh oh!

Conversation

5hv5hvnk commented Jan 28, 2022 • edited by ricardoV94 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelosthege left a comment

Choose a reason for hiding this comment

Uh oh!

ricardoV94 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

5hv5hvnk commented Jan 31, 2022

Uh oh!

ricardoV94 commented Feb 1, 2022

Uh oh!

5hv5hvnk commented Feb 4, 2022

Uh oh!

ricardoV94 commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricardoV94 commented Feb 15, 2022

Uh oh!

michaelosthege left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelosthege left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canyon289 commented Mar 13, 2022

Uh oh!

michaelosthege commented Mar 14, 2022

Uh oh!

codecov bot commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

5hv5hvnk commented Mar 14, 2022

Uh oh!

twiecki commented Mar 14, 2022

Uh oh!

5hv5hvnk commented Mar 14, 2022

Uh oh!

twiecki commented Mar 14, 2022

Uh oh!

Uh oh!

5hv5hvnk commented Jan 28, 2022 •

edited by ricardoV94

Loading

ricardoV94 commented Feb 15, 2022 •

edited

Loading

codecov bot commented Mar 14, 2022 •

edited

Loading