Fix bug when freezing_rv_and_dims after a model transformation
#7296
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In most function transforms the caller is both creating the fgraph representation and discarding it, so it's safe to mutate the fgraph in place.
ricardoV94
force-pushed
the
freeze_dims_in_rvs
branch
from
466910a
to
0c13baf
Compare
|
I think I broke something in the fgraph stuff, need to investigate |
|
Nope this was good, it's #7265 that was broken |
jessegrabowski
approved these changes
May 3, 2024
| @@ -278,12 +278,18 @@ def fgraph_from_model( | |||
| return fgraph, memo | |||
|
|
|||
|
|
|||
| def model_from_fgraph(fgraph: FunctionGraph) -> Model: | |||
| def model_from_fgraph(fgraph: FunctionGraph, mutate_fgraph: bool = False) -> Model: | |||
There was a problem hiding this comment.
Every time you call the function you use mutate_fgraph = True, should that be the default instead? Or are the instances in this PR special cases (and there are many others)
There was a problem hiding this comment.
I prefer to ask for consent
Yes, because the shared dim length was not the exact same variable used in the size of the RVs due to an extra cloning of the later. They would still be resized correctly if you updated the dim length but not frozen when you called the associated utility |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Also adds optimization to avoid cloning the fgraph when it will be discarded anyway.
Benchmarked with long models and can cut
clone_model(the simplest workflow) by 2xRelated Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pymc--7296.org.readthedocs.build/en/7296/