Refactoring the ChiSquared distribution

[larryshamalama]

This PR further addresses the ongoing issue 4686

I don't fully understand the process of creating tests, but I try to follow the previously mentioned template. If my edits are good (please let me know if I need to correct anything), my questions are:

• How can I know when to use seeded_numpy_distribution_builder or seeded_scipy_distribution_builder vs. creating my own function?
• What tests should be run in the list tests_to_run?

If there are other PRs or documentation that addresses my questions, I am happy to be pointed to them.

Thanks!

[twiecki]

Thanks for taking this on @larryshamalama!

[ricardoV94]

@larryshamalama Thanks for taking on this one.

Unfortunately it seems the chisquare RandomVariable should go on our aesara library , as it is offered by numpy. Would you like to open a PR there? It would look similar to this one: https://github.com/pymc-devs/aesara/pull/363/files, but including just the ChiSquareRV that you created here.

Then we can come back to this PR and import the op from aesara. We might still need other changes related to the fact we are inheriting from the Gamma, but we can discuss that after.

[larryshamalama]

Thanks so much for the suggestions @ricardoV94, I can do just that and then edit this PR accordingly.

[larryshamalama]

Using aesara's op now. Still unsure if I am properly inheriting from Gamma

Edit: jobs seem to be failing...

[ricardoV94]

Even after fixing the errors, the tests will still fail until a new version of Aesara is released. You should test that it passes locally for the time being

[ricardoV94]

The latest Aesara version was released, you should be able to install it (version 2.0.9) and make use of the chisquare function now.

[ricardoV94]

Hi @larryshamalama, any progress on this PR?

[larryshamalama]

Hi @ricardoV94, so sorry for the delay, this PR sat on my back burner due to organizing an upcoming conference. Let me revisit this today.

[ricardoV94]

There is no rush, just wanted to know what was the status :)

[larryshamalama]

Even after fixing the errors, the tests will still fail until a new version of Aesara is released. You should test that it passes locally for the time being

Embarrassing question... do you mind explaining how to check this? I'm new to testing in general and was not able to successfully run some test functions by copy pasting into a notebook (see here). Do you have any suggestions or tips?

[ricardoV94]

Embarrassing question... do you mind explaining how to check this? I'm new to testing in general and was not able to successfully run some test functions by copy pasting into a notebook

There is nothing embarrassing about it. First of all, the tests should now work here as we have already updated the aesara version we are using.

In any case it's very important to get comfortable running tests locally. Ideally you would run your tests from the terminal by calling pytest path/to/test file.py -k testfunction/testclass You should read the documentation here or search for some tutorials.

Alternatively you can use some IDEs like pycharm that allow you to run tests from the interface.

Copying tests to a separate notebook should only be a last resort thing (if ever advisable) because it's much more difficult to setup, and not easy to use debugging tools that you'll need when you can't easily figure out why a test is failing.

[ricardoV94]

You should also install pre-commit locally if you haven't yet. One of the failing tests is just the code style check because you didn't include a space between the division operator.

You can read our wiki to learn how to install and run it: https://github.com/pymc-devs/pymc3/wiki/PyMC3-Python-Code-Style

This way it will catch this sort of errors automatically (and fix them if it can) when you try to make a commit, saving you some time.

[larryshamalama]

so I spent some time looking into testing and the library... My newly created tests are failing but I have no idea how to fix them. Checking TestChiSquare using pytest test_distributions_random.py -k TestChiSquare gives me to following error:

============================================================================== short test summary info ============================================================================== FAILED test_distributions_random.py::TestChiSquare::test_distribution - TypeError: object.__init__() takes exactly one argument (the instance to initialize)

I checked my ChiSquare class in continuous.py, but I feel a bit stuck. Do you have any thoughts or pointers that you want to share?

[ricardoV94]

Looks good to me. Let's see if the tests pass :)

[twiecki]

Thanks @larryshamalama!

[ricardoV94]

Was this merged into v4?

[twiecki]

Damn, my bad. @ricardoV94 can you fix that?

[ricardoV94]

I don't think I would have realized it in time either. How can it be fixed, other than opening a new PR with the same content?

[larryshamalama]

Oh no, was this supposed to be merged into another branch?