refactor pareto and laplace #4691
Conversation
|
It seems like the Pareto has been half refactored already here #4593 It should be enough to remove the unused The tests in distributions_random look fine. |
Ah thank you, I just noticed that. |
| pymc_dist = pm.Pareto | ||
| pymc_dist_params = {"alpha": 3.0, "m": 2.0} | ||
| expected_rv_op_params = {"alpha": 3.0, "m": 2.0} | ||
| reference_dist_params = {"alpha": 3.0, "scale": 2.0} |
There was a problem hiding this comment.
reference_dist_params should be "b" and "scale" I think, that's why the tests are failing
There was a problem hiding this comment.
Instead of seeded_numpy_distribution_builder, I should use seeded_scipy_distribution_builder to match the params, right?
There was a problem hiding this comment.
Yes, you're right. Aesara is using scipy for the Pareto
| pymc_dist_params = {"alpha": 3.0, "m": 2.0} | ||
| expected_rv_op_params = {"alpha": 3.0, "m": 2.0} | ||
| reference_dist_params = {"alpha": 3.0, "scale": 2.0} | ||
| size = 15 |
There was a problem hiding this comment.
No need to specify size, 15 is already the default
|
@farhanreynaldo I noticed we lost the default transform of the Pareto in #4593. I pushed a fix to this PR. Make sure to pull the changes to your local repo before pushing new commits! |
| pymc_dist_params = {"mu": 0.0, "b": 1.0} | ||
| expected_rv_op_params = {"mu": 0.0, "b": 1.0} | ||
| reference_dist_params = {"loc": 0.0, "scale": 1.0} | ||
| size = 15 |
There was a problem hiding this comment.
size can be removed here too
|
I'm sorry I just realized the You can simply import the op from there no need to recreate it. I suggest you check if I missed any other op that's already implemented there. The Truncated Normal and Wald seem to be already there as well |
I am terribly sorry, I did not intend to do so. Should I run the following command before pushing new commits to the current branch? |
I just saw the link you sent me, but the |
|
I think I messed up the git merge once again... |
ricardoV94
force-pushed
the
port-distribution
branch
from
e0c6b97
to
fbd2103
Compare
|
@farhanreynaldo No problem. I'm not very good at git either :p I think I fixed the commit history in this branch. Let me know if there was anything else you wanted to change or if I can go ahead and merge? |
Yes, you can merge it. Thank you so much for your help on this PR! |
|
@farhanreynaldo Thanks so much for taking on this! Looking forward to your next PR! |
* refactor pareto * refactor laplace * Reintroduce `Pareto` default transform Co-authored-by: Farhan Reynaldo <farhanreynaldo@gmail.com> Co-authored-by: Ricardo <ricardo.vieira1994@gmail.com>
This PR closes subset of #4686.
It is still unclear to me on the default shape of the
RandomVariableclass, so I might need suggestion on that part. Thetests_to_runparameter ontest_distribution_random.pyalso baffles me, your input is much appreciated.