Skip to content

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 fonnesbeck requested a review from ricardoV94 January 7, 2023 21:04
@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

@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
fonnesbeck and others added 4 commits January 29, 2023 21:19
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
@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