Skip to content
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

Make VI (posterior) mean and std accessible as a structured xarray #6387

Merged
merged 23 commits into from
Jan 30, 2023
Merged

Make VI (posterior) mean and std accessible as a structured xarray #6387

merged 23 commits into from
Jan 30, 2023

Conversation

fonnesbeck
Copy link
Member

What is this PR about?

Moved from #6086 so that merge conflicts could be fixed. See original PR for details.

@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Merging #6387 (c174630) into main (00d2675) will decrease coverage by 9.87%.
The diff coverage is 94.91%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6387      +/-   ##
==========================================
- Coverage   94.79%   84.92%   -9.87%     
==========================================
  Files         148      148              
  Lines       27730    27785      +55     
==========================================
- Hits        26287    23597    -2690     
- Misses       1443     4188    +2745     
Impacted Files Coverage Δ
pymc/variational/opvi.py 87.46% <87.50%> (+0.35%) ⬆️
pymc/tests/variational/test_inference.py 99.20% <100.00%> (+0.12%) ⬆️
pymc/tests/variational/test_updates.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/distributions/test_continuous.py 0.00% <0.00%> (-99.78%) ⬇️
pymc/tests/backends/test_arviz.py 0.00% <0.00%> (-99.05%) ⬇️
pymc/tests/distributions/test_multivariate.py 36.44% <0.00%> (-63.01%) ⬇️
pymc/variational/updates.py 37.93% <0.00%> (-54.19%) ⬇️
pymc/distributions/multivariate.py 57.77% <0.00%> (-34.51%) ⬇️
pymc/distributions/continuous.py 66.66% <0.00%> (-31.02%) ⬇️
pymc/data.py 65.18% <0.00%> (-26.59%) ⬇️
... and 11 more

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variational tests were moved into tests/variational. I am not sure where this one should go, perhaps @Armavica can weigh in?

@Armavica
Copy link
Member

I think test_fit_data makes sense in tests/variational/test_inference.py, as for test_fit_data_coords and its fixtures I think it would be fine too…

@twiecki
Copy link
Member

twiecki commented Dec 30, 2022

@fonnesbeck Any update on this?

@fonnesbeck
Copy link
Member Author

OK, tests moved over.

@fonnesbeck
Copy link
Member Author

@ricardoV94 are you happy with this?

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a deprecated method and the use of the | operator for dicts which is not supported in our oldest Python dependency (test is failing)

Otherwise LGTM

pymc/variational/opvi.py Outdated Show resolved Hide resolved
@@ -977,7 +978,7 @@ def symbolic_random(self):

@pytensor.config.change_flags(compute_test_value="off")
def set_size_and_deterministic(
self, node: Variable, s, d: bool, more_replacements: dict | None = None
self, node: Variable, s, d: bool, more_replacements: dict = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self, node: Variable, s, d: bool, more_replacements: dict = None
self, node: Variable, s, d: bool, more_replacements: Optional[dict] = None

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, the pre-commit hook reverts this back to dict | None = None. A little too agressive!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can commit with --no-verify to skip the pre-commit

@michaelosthege michaelosthege changed the title make vi (posterior) mean and std accessible as a structured xarray Make VI (posterior) mean and std accessible as a structured xarray Jan 28, 2023
pymc/variational/opvi.py Outdated Show resolved Hide resolved
pymc/variational/opvi.py Outdated Show resolved Hide resolved
pymc/variational/opvi.py Outdated Show resolved Hide resolved
pymc/variational/opvi.py Outdated Show resolved Hide resolved
fonnesbeck and others added 4 commits January 29, 2023 21:19
Co-authored-by: Michael Osthege <michael.osthege@outlook.com>
Co-authored-by: Michael Osthege <michael.osthege@outlook.com>
Co-authored-by: Michael Osthege <michael.osthege@outlook.com>
Co-authored-by: Michael Osthege <michael.osthege@outlook.com>
@fonnesbeck fonnesbeck merged commit 26048a4 into pymc-devs:main Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants