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

Remove t suffix from Model methods #5859

Closed
ricardoV94 opened this issue Jun 6, 2022 · 9 comments · Fixed by #5863 or #6237
Closed

Remove t suffix from Model methods #5859

ricardoV94 opened this issue Jun 6, 2022 · 9 comments · Fixed by #5863 or #6237
Milestone

Comments

@ricardoV94
Copy link
Member

ricardoV94 commented Jun 6, 2022

model.logpt, model.dlogpt and model.d2logpt would be a bit more intuitive without the t suffix. Historically that was done to distinguish the tensor graphs from the compiled functions, but those are now obtained via model.compile_logp, model.compile_dlogp and model.compile_d2logp.

@twiecki
Copy link
Member

twiecki commented Jun 6, 2022

I never knew what the t stood for, so yes, best to remove.

@ricardoV94
Copy link
Member Author

If anyone wants to take a go, this should be a simple deprecation PR:

  1. Rename the new methods to pymc.model.Model without the t suffix,
  2. Keep the old ones as a simple wrapper that issues a FutureWarning in between.
  3. Change all the tests to make use of the new names,
  4. Add some tiny tests to make sure the deprecation is being issued,
  5. Create a new Github Issue to remove the deprecated names in v4.2

@cuchoi
Copy link
Member

cuchoi commented Jun 7, 2022

I can take this one if that's ok!

@twiecki
Copy link
Member

twiecki commented Jun 7, 2022

@cuchoi Great, just open a draft PR so that people see you're working on it.

@cuchoi
Copy link
Member

cuchoi commented Jun 7, 2022

Should I update datalogpt, varlogpt, observedlogpt, and potentiallogpt, distributions.joint_logpt as well?

@ricardoV94
Copy link
Member Author

Should I update datalogpt, varlogpt, observedlogpt, and potentiallogpt, distributions.joint_logpt as well?

Yes! I forgot about those

@cuchoi
Copy link
Member

cuchoi commented Jun 8, 2022

Cool. Should I update documentation such as docs/source/contributing/developer_guide.rst and docs/source/learn/core_notebooks/pymc_aesara.ipynb?

@ricardoV94
Copy link
Member Author

Cool. Should I update documentation such as docs/source/contributing/developer_guide.rst and docs/source/learn/core_notebooks/pymc_aesara.ipynb?

Yes

@ricardoV94 ricardoV94 added this to the v4.2.0 milestone Jun 14, 2022
@ricardoV94
Copy link
Member Author

Opening this as a reminder to deprecate in a future major release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants