daft-dev / daft

Render probabilistic graphical models using matplotlib
https://docs.daft-pgm.org
MIT License
675 stars 118 forks source link

Upgrades #123

Closed varunagrawal closed 3 years ago

varunagrawal commented 3 years ago

This PR adds 2 upgrades:

  1. Replace the deprecated np.float with np.float64 as recommended by numpy.
  2. In PGM.render, return the figure as well as the axes, so that a user can use the figure to set various properties like the title and window label.
varunagrawal commented 3 years ago

I also have a question about the reset_figure method. Should we be using plt.clf instead of plt.close so that any user set properties on the figure aren't lost when calling PGM.show?

dfm commented 3 years ago

Thanks for this! I'm definitely keen to merge the float64 changes, but I'm less excited about the change to what render returns. I agree that it should return the figure (as in that would be a better design!), but since development is so slow on this package, I'd prefer not to break backwards compatibility. It's not too hard to access the figure already using pgm.figure(); would that be sufficient for your needs? An alternative would be to add a return_figure argument to render with a deprecation warning so that we can phase out the current behavior.

How about you revert the return change in this PR (I'm happy to merge that!) and open two others with the clf (that seems like a great idea!) and the option to return the figure (if you want)?

varunagrawal commented 3 years ago

Hi @dfm, thanks a lot for your prompt response.

I understand the need not to break backwards compatibility, so I'll revert the changes to render.

Using pgm.figure() doesn't work for me since render clears the current figure before plotting. Then again, after sleeping on the problem, I should be able to use it by calling render() and plt.show separately rather than the single pgm.show.

Given it's already possible to access the figure, and that clf will solve my major issue of figure properties being obliterated, I love your recommendations and will shoot for the first 2 PRs.

varunagrawal commented 3 years ago

@dfm also, can you please release v0.1.1 on PyPI soon?

dfm commented 3 years ago

@varunagrawal: Thanks and absolutely!