Skip to content

Refactor Factor properties #5320

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

Merged
merged 9 commits into from
Jan 14, 2022
Merged

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jan 7, 2022

This PR deprecates most of the Factor properties, and gives the following default methods:

  • compile_logp(self, vars=None, jacobian=True, sum=True):
  • compile_dlogp(self, vars=None, jacobian=True):
  • compile_d2logp(self, vars=None, jacobian=True):

The prefix compile is there to make it obvious this is a costly operation, and that the results should be cached once and reused afterwards. Also this makes it easier to deprecate the old properties, which couldn't easily be changed into methods.

It also deprecates LoosePointFunc and renames FastPointFunc to PointFunc which is now the default. This closes #5318.

Finally this PR also renames pm.logpt to pm.joint_logpt, which is now the main bridge between PyMC and Aeppl. The reason for this is that pm.logpt has diverged quite a lot from what it was doing in V3, and because of that it has a lot of ugly internal logic that should be handled by the Model object instead (involving trying to find value variables, transforms and scaling in the tags of the variables). The rename will hopefully help break the dependency that's there for backwards compat and facilitate refactoring in the future, such as #5033.


It would be nice to find a nice way to transition from Model.logpt being a property -> method that can accept vars, jacobian and sum as an argument. Then we can remove the more verbose logp_elemwiset, which is temporarily playing this more flexible role, in service of all other methods and properties Decided to break away immediately following discussion in #5333

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #5320 (a1e30eb) into main (1b32070) will increase coverage by 0.11%.
The diff coverage is 91.39%.

❗ Current head a1e30eb differs from pull request most recent head 75ea232. Consider uploading reports for the commit 75ea232 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5320      +/-   ##
==========================================
+ Coverage   80.23%   80.34%   +0.11%     
==========================================
  Files          89       89              
  Lines       14856    14805      -51     
==========================================
- Hits        11919    11895      -24     
+ Misses       2937     2910      -27     
Impacted Files Coverage Δ
pymc/backends/arviz.py 90.28% <ø> (ø)
pymc/distributions/__init__.py 100.00% <ø> (ø)
pymc/model_graph.py 85.51% <ø> (ø)
pymc/step_methods/sgmcmc.py 0.00% <0.00%> (ø)
pymc/tuning/starting.py 92.56% <83.33%> (+9.36%) ⬆️
pymc/step_methods/metropolis.py 83.05% <85.71%> (ø)
pymc/model.py 85.93% <92.64%> (+1.70%) ⬆️
pymc/backends/base.py 86.56% <100.00%> (ø)
pymc/distributions/logprob.py 96.36% <100.00%> (-0.13%) ⬇️
pymc/sampling.py 86.31% <100.00%> (-0.11%) ⬇️
... and 9 more

@ricardoV94 ricardoV94 marked this pull request as draft January 7, 2022 13:33
@ricardoV94 ricardoV94 force-pushed the joint_logpt branch 5 times, most recently from 26ea8ce to 354cca8 Compare January 7, 2022 13:52
@ricardoV94 ricardoV94 requested a review from twiecki January 7, 2022 13:54
@ricardoV94 ricardoV94 marked this pull request as ready for review January 7, 2022 14:02
@ricardoV94 ricardoV94 force-pushed the joint_logpt branch 2 times, most recently from 934a5bb to 8c47f83 Compare January 7, 2022 17:34
@ricardoV94 ricardoV94 modified the milestone: v4.0.0b3 Jan 7, 2022
Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

Sorry about that nitpick, but given how much code calls these properties/methods, could you add some type hints?
Even incomplete type hints here can give a lot of mypy coverage, right?

@ricardoV94 ricardoV94 added this to the v4.0.0b3 milestone Jan 8, 2022
@ricardoV94 ricardoV94 marked this pull request as draft January 9, 2022 15:51
@ricardoV94 ricardoV94 force-pushed the joint_logpt branch 4 times, most recently from 6cf2209 to bf4c982 Compare January 10, 2022 10:07
@ricardoV94 ricardoV94 marked this pull request as ready for review January 10, 2022 10:10
@ricardoV94 ricardoV94 force-pushed the joint_logpt branch 2 times, most recently from 96bc0dc to 42e3eaa Compare January 13, 2022 15:03
@twiecki
Copy link
Member

twiecki commented Jan 13, 2022

This is shaping up to be a really nice refactor of some quite obscure code.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jan 13, 2022

This is shaping up to be a really nice refactor of some quite obscure code.

Hopefully one will also be less lost when doing autocomplete on a Model to find a method xD

@ricardoV94 ricardoV94 marked this pull request as ready for review January 13, 2022 16:37
* compiled functions like `dlogp` are now called `compile_*`
* `logp_elemwiset` -> `logpt(sum=False)`
* `*_no_jacobian` graphs and functions are now obtained via the keyword `jacobian=False`
* Value variables are no longer passed to `model.compile_fn` by default
* Rename `logpt` to `joint_logpt`
This also fixes a bug where graphs with NotImplemented gradients were not detected before deciding on optimization method
This would fail when the chains do not have the same length, resulting in the marginal_likelihood being wrapped in a numpy array of `object` dtype. This led the nanmean call to fail with an AttributeError
@michaelosthege
Copy link
Member

I'm open to having docstring link to docstrings of the other properties/methods.
We can probably make the necessary changes all as suggestions and have one commit for them?

@ricardoV94
Copy link
Member Author

I'm open to having docstring link to docstrings of the other properties/methods. We can probably make the necessary changes all as suggestions and have one commit for them?

Yeah I think the compile_* is better of just directing to the respective methods for the meaning of the variables. Feel free to make changes and force-push to this branch! Won't have time to work on this until next week

@michaelosthege michaelosthege merged commit 2f83818 into pymc-devs:main Jan 14, 2022
@ricardoV94 ricardoV94 deleted the joint_logpt branch January 20, 2022 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate LoosePointFunc and make FastPointFunc the default
3 participants