Skip to content

Remove t suffix from Model methods #5863

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 30 commits into from
Jun 14, 2022
Merged

Conversation

cuchoi
Copy link
Member

@cuchoi cuchoi commented Jun 7, 2022

Closes #5859

To-do:

  • Rename model.logpt, model.dlogpt and model.d2logpt to pymc.model.Model without the t suffix. Keep the old ones as a simple wrapper that issues a FutureWarning in between
  • Update model.datalogpt, model.varlogpt, model.observedlogpt, and model.potentiallogpt, distributions.joint_logpt as well
  • Change all the tests to make use of the new names
  • Add some tiny tests to make sure the deprecation is being issued
  • Update documentation docs/source/contributing/developer_guide.rst and docs/source/learn/core_notebooks/pymc_aesara.ipynb

@twiecki twiecki marked this pull request as draft June 7, 2022 14:58
@twiecki twiecki changed the title [WIP] Remove t suffix from Model methods Remove t suffix from Model methods Jun 7, 2022
@twiecki twiecki requested a review from ricardoV94 June 7, 2022 14:59
@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #5863 (a33cd0b) into main (a5f3e45) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5863      +/-   ##
==========================================
+ Coverage   89.45%   89.50%   +0.04%     
==========================================
  Files          73       73              
  Lines       13234    13267      +33     
==========================================
+ Hits        11839    11874      +35     
+ Misses       1395     1393       -2     
Impacted Files Coverage Δ
pymc/backends/arviz.py 90.28% <ø> (ø)
pymc/distributions/__init__.py 100.00% <ø> (ø)
pymc/distributions/continuous.py 97.98% <ø> (ø)
pymc/smc/smc.py 96.45% <ø> (ø)
pymc/distributions/logprob.py 97.72% <100.00%> (+0.07%) ⬆️
pymc/model.py 87.87% <100.00%> (+0.61%) ⬆️
pymc/sampling.py 82.41% <100.00%> (ø)
pymc/sampling_jax.py 96.93% <100.00%> (ø)
pymc/step_methods/metropolis.py 83.36% <100.00%> (ø)
pymc/step_methods/mlda.py 96.37% <100.00%> (ø)
... and 3 more

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@cuchoi cuchoi marked this pull request as ready for review June 8, 2022 15:00
@cuchoi
Copy link
Member Author

cuchoi commented Jun 8, 2022

Should I add/replicate test for the t ending functions? I only added FutureWarning tests for those but no functionality tests (for example if there is a typo and dlogpt calls logpt -- I double checked but you never know :))

@ricardoV94
Copy link
Member

ricardoV94 commented Jun 8, 2022

Should I add/replicate test for the t ending functions? I only added FutureWarning tests for those but no functionality tests (for example if there is a type and dlogpt and calls logpt -- I double checked but you never know :))

If you want you can try to monkey patch the methods to make sure what was called internally, but otherwise there's no need. I'll review carefully with some fresh eyes to confirm it looks alright ;)

@michaelosthege
Copy link
Member

This looks like something we should fast-track into a 4.0.1 release to minimize the friction for people migrating their logp* calls.

@ricardoV94 can you review it?

@ricardoV94
Copy link
Member

I'll review it on Monday

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.

Looks great, left some suggestions and highlighted one missed property with t suffix.

RELEASE-NOTES.md Outdated
Comment on lines 116 to 117
- `logpt`, `logpt_sum`, `logp_elemwiset` and `nojac` variations were removed. Use `Model.logp(jacobian=True/False, sum=True/False)` instead.
- `dlogp_nojact` and `d2logp_nojact` were removed. Use `Model.dlogp` and `d2logp` with `jacobian=False` instead.
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't change the old Release Notes as this refer to the already released V4.0.0. Instead you should add a new release note in the most recent section explaining the changes from t suffix to no suffix.

Copy link
Member

Choose a reason for hiding this comment

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

This weekend I already created a 4.0.1 section in the release notes.
You'll have to rebase, otherwise there will be git conflicts..

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.

Looks great 👍

@cuchoi
Copy link
Member Author

cuchoi commented Jun 13, 2022

I rebased but now some changes appear done not by me appear as they were new, is that ok? Do you know how to fix it? Fixed

Also, should I add this change to the Release Notes of 4.0.1? Added release notes

@cuchoi cuchoi changed the base branch from main to add-custom-op-doc June 13, 2022 21:30
@cuchoi cuchoi changed the base branch from add-custom-op-doc to main June 13, 2022 21:30
@twiecki twiecki merged commit 5d003dc into pymc-devs:main Jun 14, 2022
@twiecki
Copy link
Member

twiecki commented Jun 14, 2022

Thanks @cuchoi!

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.

Remove t suffix from Model methods
4 participants