Skip to content

Fix dim lengths created from coords or ConstantData #5751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
michaelosthege opened this issue May 6, 2022 · 7 comments · Fixed by #5763
Closed

Fix dim lengths created from coords or ConstantData #5751

michaelosthege opened this issue May 6, 2022 · 7 comments · Fixed by #5763

Comments

@michaelosthege
Copy link
Member

In this comment we proposed to make dimensions created from coords non-resizeable, because it would prevent users from running into issues like #5181.

What needs to be done

In

pymc/pymc/model.py

Lines 1070 to 1076 in dd23c90

def add_coord(
self,
name: str,
values: Optional[Sequence] = None,
*,
length: Optional[Variable] = None,
):
the set_coord method could take another kwarg resizable: bool=False or fixed: bool=True.

Then we can actively turn off re-sizing of dimensions that are created from ConstantData or coords.

@ricardoV94
Copy link
Member

ricardoV94 commented May 6, 2022

Let's remember to add #5181 as a test

@LukeLB
Copy link
Contributor

LukeLB commented May 13, 2022

Hi guys just been looking at how this can be implemented. Can I check my understanding first... So this line:

self._dim_lengths[name] = length or aesara.shared(len(values))

Is causing the problem due to the aesara.shared(len(values)) being resizeable. So the sugestion here is to create a new kwarg which we can use in an if statement, something like:

if fixed:
    self._dim_lengths[name] = length or aesara.tensor.constant(len(values))
else: 
    self._dim_lengths[name] = length or aesara.shared(len(values))

@michaelosthege
Copy link
Member Author

@LukeLB yes, that's the idea

@canyon289
Copy link
Member

@LukeLB Did you want to take a shot at fixing this, and if so when were you planning on doing so? I ask as this is holding up the Bambi migration to V4 so we want to get fixed soon here!

@LukeLB
Copy link
Contributor

LukeLB commented May 14, 2022

@canyon289 I've got time today so I'll have a crack at it.

@canyon289
Copy link
Member

Sounds great! Looking forward to the PR and thank you. Let us know if you need any help.

@LukeLB
Copy link
Contributor

LukeLB commented May 14, 2022

@canyon289 just put in a WIP-PR as I'm heading out and was having some problems with locally failing tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants