Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Generalized Extreme Value Distribution #4

Closed
wants to merge 0 commits into from

Conversation

ccaprani
Copy link
Contributor

@ccaprani ccaprani commented Feb 2, 2022

Follows from pymc-devs/pymc#5085

@ricardoV94 This is an initial not-working commit to see if you agree with the structure. There are a couple of things worth discussing:

  1. I don't think this repo needs to replicate pymc itself here - any existing distributions/functions/utilities there should just be imported here.
  2. Due to the nature of this repo, I think each experimental addition should have it's own file - hence what I've gone with in __init__
  3. I've cherry picked the imports from pymc, aesera, etc as required for GenExtreme; however, I wonder if it would be better to have a generic set of imports that all/most experimental distributions could just use (even if the rather blunt star import).
  4. I presume tests should be part of this and have added such. The structure though will be different to pymc and perhaps just a lower level pytest format is acceptable?
  5. Is there anything else I've missed? In particular, I think the imports should "just work" once there is a pymc installed, right?

Can you review what I've done so far, and if it's on the right track I'll get it working.

Once we agree the plumbing, I'll update the Case Study example PR accordingly.

Cheers!

@ricardoV94
Copy link
Member

  1. I don't think this repo needs to replicate pymc itself here - any existing distributions/functions/utilities there should just be imported here.

Definitely!

  1. Due to the nature of this repo, I think each experimental addition should have it's own file - hence what I've gone with in __init__

I don't have a strong opinion here. Feel free to set the standard

  1. I've cherry picked the imports from pymc, aesera, etc as required for GenExtreme; however, I wonder if it would be better to have a generic set of imports that all/most experimental distributions could just use (even if the rather blunt star import).

Seems fine to me to use explicit imports in every file

  1. I presume tests should be part of this and have added such. The structure though will be different to pymc and perhaps just a lower level pytest format is acceptable?

As you wish. The advantage of keeping a similar strategy is that it would make upgrading experimental features to PyMC easier

  1. Is there anything else I've missed? In particular, I think the imports should "just work" once there is a pymc installed, right?

Have you ran the tests locally?

Can you review what I've done so far, and if it's on the right track I'll get it working.

I think you are exactly on the right track

Once we agree the plumbing, I'll update the Case Study example PR accordingly.

Awesome 👍

@ccaprani
Copy link
Contributor Author

ccaprani commented Feb 4, 2022

@ricardoV94 I've updated per discussion and pmx now works as a package, and at least the basic structure is now in place for further contributions. I've also updated the related example pymc-devs/pymc-examples#241 which you can try locally perhaps, but it works fine for me.

One thing: the tests won't run locally because of pydata/xarray#6039 - not sure if you can replicate?

Please review - I suppose there are two reviews in a way...1) the more general pmx structure; 2) the specifics of this distribution, GenExtreme.

@ccaprani
Copy link
Contributor Author

Something went wrong with my last commit - this new one has those comments addressed. Thanks!

Comment on lines 211 to 218
def get_moment(value_var, size, mu, sigma, xi):
r"""
Using the mode, as the mean can be infinite when :math:`\xi > 1`
"""
mode = at.switch(
at.isclose(xi, 0), mu, mu + sigma * (at.pow(1 + xi, -xi) - 1) / xi
)
return at.full(size, mode, dtype=aesara.config.floatX)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the last thing, can you add tests for the moment? You can borrow the helper from test_distribution_moments in the main repo

)


class TestMatchesScipyX(TestMatchesScipy):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this rerun all the tests in the baseclass? In a discussion with @canyon289 it seemed like that was the case

@ccaprani
Copy link
Contributor Author

ccaprani commented Jul 6, 2022

@ricardoV94 After a delay, I'm trying to close this out. To the outstanding questions:

  • test for moment added; note it is using mode not mean.
  • TestMatchesScipyX was intended so the method could be just dropped into the main pymc once confirmed stable.

There's been quite a few changes across both repos since March and the tests aren't succeeding locally now for me. If they fail on push, then reckon I'll need your help to close it out.

More broadly; we've been using this distribution a lot in our work and this implementation is very stable once data is properly centered. It would be a good addition to the main repo.

@ccaprani
Copy link
Contributor Author

Continued as #84 due to rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants