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 Plots & Diagnostics from PyMC Code Base #4371

Closed
AlexAndorra opened this issue Dec 22, 2020 · 13 comments · Fixed by #4397
Closed

Remove Plots & Diagnostics from PyMC Code Base #4371

AlexAndorra opened this issue Dec 22, 2020 · 13 comments · Fixed by #4397
Assignees
Milestone

Comments

@AlexAndorra
Copy link
Contributor

Plots and diagnostics are still accessible via PyMC3 but are actually implemented by ArviZ now. Although this was a good call when ArviZ was still nascent, now this causes confusion for new comers and overhead for devs, who have to mirror the functions in ArviZ, which is more work when ArviZ adds or changes functions (e.g pm.hpd vs. az.hdi).

To make all this more robust and easier to maintain, we should completely remove plots and diagnostics from the PyMC code base and rely on ArviZ, since the latter library is now much more known and flies on its own (yeah, I'm a part-time poet).

The coming 4.0 major release is an opportunity to make this breaking change -- plus, it'll make the code base cleaner and leaner 👌 Feel free to comment if you wanna take on this issue 🖖

@MarcoGorelli
Copy link
Contributor

yeah, I'm a part-time poet

Must come from all that listening to Baba Brinkman ;)

@CloudChaoszero
Copy link
Contributor

+1 lol ^^^


Hey there @AlexAndorra, is this an Issue I can participate in?

Also, when do you think the ETA/Due Date for the coming 4.0 major release would be?

@AlexAndorra
Copy link
Contributor Author

Hey there @AlexAndorra, is this an Issue I can participate in?

Of course @CloudChaoszero ! I just assigned it to you. Feel free to ask questions 😉

Also, when do you think the ETA/Due Date for the coming 4.0 major release would be?

We usually don't give hard deadlines but are all commited and working to make that happen as soon as possible 🙂
Getting new contributions is definitely helpful in that regard, so thank you!

@CloudChaoszero
Copy link
Contributor

Great! I'll reach out if I have questions etc @AlexAndorra.

@CloudChaoszero
Copy link
Contributor

@AlexAndorra Hey there!

I was looking looking at the codebase, particularly Pymc's plots/init.py, and I saw functions like compareplot and traceplot borrow functionality from arviz, but do a couple of work arounds.

Example

These workarounds tries to relabel kwargs parameters, and other related items like that.

I was wondering should the kwargs param renaming be copied to arviz repo, or should the expected breakchanges for v4.0 assume users rename the params themselves?

@AlexAndorra
Copy link
Contributor Author

AlexAndorra commented Jan 2, 2021

Yes, you can just remove all this @CloudChaoszero, as the idea is to completely delegate to ArviZ.
As a sidenote, we just switched compact to True by default in az.plot_trace anyway 😉

@CloudChaoszero
Copy link
Contributor

Yes, you can just remove all this @CloudChaoszero, as the idea is to completely delegate to ArviZ.
As a sidenote, we just switched compact to True by default in az.plot_trace anyway 😉

Sounds great, thanks!

@CloudChaoszero
Copy link
Contributor

@AlexAndorra Quick Question: Should customer function plot_posterior_predictive_glm Source be removed as well, and users would try to adopt their function to arviz? I only see arviz's plot_posterior Source right now.

  • Note: I can try to make a migration PR for it into Arviz, if necessaary

@AlexAndorra
Copy link
Contributor Author

Ugh, that's a good question -- we discussed it recently with @MarcoGorelli and decided to keep it around IIRC. But the 4.0 release is a good opportunity to remove it and completely switch to ArviZ.

plot_posterior_predictive_glm can probably be easily replicated with az.plot_ppc 🤔
Curious about what @OriolAbril think: is it worth it to migrate it to ArviZ, or are ArviZ's current methods enough?

@OriolAbril
Copy link
Member

I don't know how plot_posterior_predictive_glm works internally, but my guess is that is should be kept in PyMC3 until there are regression plots in ArviZ, that is when arviz-devs/arviz#313 and arviz-devs/arviz#512 are solved. Maybe in this year's GSoc?

How does that sound? It may not match the 3-4 timeline though

@MarcoGorelli
Copy link
Contributor

plot_posterior_predictive_glm is just a few lines of code - if you have e.g. a model of the form y = Normal(a + bx, sigma) then it takes samples from the trace and plots a + bx for the various samples. Granted my knowledge here is really limited, but given that in the above example it disregards the values of sigma and just plots the regression lines a + bx, is it fair to call it "posterior predictive"?

@AlexAndorra
Copy link
Contributor Author

should be kept in PyMC3 until there are regression plots in ArviZ

Agreed @OriolAbril, let's do that then -- if this function proves hard to maintain because of v4 changes though, we'll probably remove it before ArviZ has regression functionalities.

is it fair to call it "posterior predictive"?

Good catch @MarcoGorelli ! I think these are indeed no posterior predictive samples but rather predictive pushforward samples. As the function is meant to disapear soon though (see my answer above), let's not change the name, as that would be a breaking change for just a few months

@CloudChaoszero
Copy link
Contributor

Sounds good, I'll update the PR accordingly!

@AlexAndorra AlexAndorra modified the milestones: 4.0.0, vNext (3.11.0) Jan 17, 2021
@AlexAndorra AlexAndorra linked a pull request Jan 17, 2021 that will close this issue
5 tasks
CloudChaoszero added a commit to CloudChaoszero/pymc-examples that referenced this issue Jan 18, 2021
* Related to issue pymc-devs/pymc#4371

🎨 Replace Pymc3 plots w/arviz plots and sigma param change

🎨 Update plots to arviz dependency instead of pymc3 dependency

🎨 Update plots to arviz dependency instead of pymc3 dependency

🎉 Finish residual replacement for Pymc3 plots to arviz & sigma param change
CloudChaoszero added a commit to CloudChaoszero/pymc-examples that referenced this issue Jan 18, 2021
* Related to issue pymc-devs/pymc#4371

🎨 Replace Pymc3 plots w/arviz plots and sigma param change

🎨 Update plots to arviz dependency instead of pymc3 dependency

🎨 Update plots to arviz dependency instead of pymc3 dependency

🎉 Finish residual replacement for Pymc3 plots to arviz & sigma param change
AlexAndorra pushed a commit that referenced this issue Jan 19, 2021
* 🎉 Start removing Diagnostics & Plots in PyMC3 development

🔥 Remove arviz plots
* Remove directly imported arviz plots used in pymc3 plots

🔥 Remove all plots from PyMC3 Plots module

🔥 Remove PyMC3 plots references in Docs

🎨 Mention Plotting & Diagnostics in API page and remove plots reference in __init__.py

⏪ Revert posterior_plot function, test, and docs

🎨 Add deprecation warning to posterior_plot function

🎨 Add context on plot import and import back into __init__.py

✏️ Add warning and details of posterior_plot added

Update docs/source/api/plots.rst

Co-authored-by: Alexandre ANDORRA <andorra.alexandre@gmail.com>

Update docs/source/api/plots.rst

Co-authored-by: Alexandre ANDORRA <andorra.alexandre@gmail.com>

Update pymc3/plots/__init__.py

Co-authored-by: Alexandre ANDORRA <andorra.alexandre@gmail.com>

Update pymc3/plots/__init__.py

Co-authored-by: Alexandre ANDORRA <andorra.alexandre@gmail.com>

✏️ Update docs to add stats.rst details

✏️ Minor docs notation for posterioplot function(s)

📝 Add breakline before docstring title

* ✏️ small typo and remove summary in plots.rst
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants