liesel-devs / liesel

A probabilistic programming framework
https://liesel-project.org
MIT License
38 stars 2 forks source link

Fixes unexpected behaviour in plot_vars when using an intermediate Calc #168

Closed GianmarcoCallegher closed 3 months ago

jobrachem commented 7 months ago

Hey @GianmarcoCallegher thanks for the PR! Could you provide some of your testing code for me to run?

GianmarcoCallegher commented 5 months ago

Before providing any example test, I will try to follow Hannes' suggestion in #89

jobrachem commented 5 months ago

Alright, that would be nice :) I'll then convert this PR back to draft status for the time being. Just re-request review once it is ready.

wiep commented 4 months ago

Why was this closed? No longer relevant, new PR?

GianmarcoCallegher commented 4 months ago

My bad. I just wanted to delete the branch but obviously it also closed the PR. I reopened it

GianmarcoCallegher commented 4 months ago

@jobrachem it is ready for the review. I didn't adopt the solution proposed in #89 (probably because it doesn't work). This is a test file:

tests_file.py.zip

jobrachem commented 3 months ago

Thanks @GianmarcoCallegher , the plots in the example script look good!

Example 1:

image

Grey line from sigma_sq to rent as expected.

Example 2:

image

Grey line from sigma_sq to sigma and from sigma to rent as expected. The black lines from b and a to sigma_sq are correct, but will likely be omitted once #174 progresses.

Example 2, turning sigma from a Var into a pure Calc

image

Grey line from sigma_sq to rent as expected.

Conclusion

Everything looks good to me! Just one request: Would you already add a line about this change to the changelog?

GianmarcoCallegher commented 3 months ago

Thanks @GianmarcoCallegher , the plots in the example script look good!

Example 1:

image

Grey line from sigma_sq to rent as expected.

Example 2:

image

Grey line from sigma_sq to sigma and from sigma to rent as expected. The black lines from b and a to sigma_sq are correct, but will likely be omitted once #174 progresses.

Example 2, turning sigma from a Var into a pure Calc

image

Grey line from sigma_sq to rent as expected.

Conclusion

Everything looks good to me! Just one request: Would you already add a line about this change to the changelog?

Thanks a lot. Done it ❤️