search

daft-dev / daft

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

clf for reset figure #124

Closed varunagrawal closed 3 years ago

varunagrawal commented 3 years ago

Support to clear figure instead of closing it for reset_figure.

This also enables better efficiency since we are not de-allocating and re-allocating memory each time.

dfm commented 3 years ago

For reasons that I don't totally understand, this breaks all the rendering. For example, the examples/classic.py example renders this before merging:

classic

and this after:

classic

I'll dig into that and add some tests, but perhaps you can take a stab at seeing what's happening too?

dfm commented 3 years ago

I actually think this is related to the automatic shape inference

https://github.com/daft-dev/daft/blob/9da2b56d551ac0cea9fcad6963dfa262b04dd831/daft.py#L387-L418

and while it might be possible to get the logic to work, unless its simple, I'm inclined to skip this!

varunagrawal commented 3 years ago

Okay, I'm a bit busy with some issues, so I'll take a look at this over the weekend? I'll ensure it works flawlessly.

dfm commented 3 years ago

Amazing - thanks and no rush! Keep me posted.

varunagrawal commented 3 years ago

@dfm I can't seem to figure out the issue. The test fails with the error message AssertionError: Test generated 2 images but there are 1 baseline images which I guess is because a new figure window is being generated. The way to fix this is to provide a figure number but that's beyond the scope of this PR and an unnecessary change for a small benefit.

Moreover, I found this [SO answer]()https://stackoverflow.com/a/46957388/1236990 that mentions how plt prefers plt.close. For this reason, I am closing this.

dfm commented 3 years ago

@varunagrawal: thanks for your time looking into this!!