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

Fix weibull lcdf #2811

Closed
wants to merge 42 commits into from
Closed

Fix weibull lcdf #2811

wants to merge 42 commits into from

Conversation

spinkney
Copy link
Collaborator

@spinkney spinkney commented Sep 8, 2022

Fix for #2810.

Release notes

Makes the Weibull cdf/lcdf more stable by using log1m_exp function.

Checklist

  • Math issue update weibull lcdf/cdf to handle 0 as an input #2810

  • Copyright holder: Sean Pinkney

    The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
    - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
    - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

  • the basic tests are passing

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • dependencies checks pass, (make test-math-dependencies)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)
  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

@spinkney spinkney closed this Sep 8, 2022
@spinkney spinkney reopened this Sep 9, 2022
spinkney and others added 2 commits September 9, 2022 04:55
Update weibull_lcdf.hpp

remove unnecessary logic

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1
@spinkney
Copy link
Collaborator Author

@rok-cesnovar I don't know how to fix the expression tests for changes up to commit 9c1f7ac (2nd to last commit). Though the changes are minimal. For the final commit, I ended up following the neg_binomial_2_l/cdf files which meant overhauling the files. In my opinion, the file is more readable but would like you or others to take a look. There aren't great tests for these functions so my plan is to add them as well.

@nhuurre for y values of 0 I have the function returning 0 for the cdf and negative infinity for the lcdf. I believe the cdf is right. I'm wondering if the lcdf is correct or if it should also return 0.

@nhuurre
Copy link
Collaborator

nhuurre commented Sep 10, 2022

Negative infinity for the lcdf is correct. The derivatives should be well-defined (and zero) when alpha > 1.

@spinkney
Copy link
Collaborator Author

Negative infinity for the lcdf is correct. The derivatives should be well-defined (and zero) when alpha > 1.

Shoot, I was hoping it was for all alpha because this raises a question about what we do with the pow() function. In the case of the model in the discourse post I get the same answer when I return a 0 for all alpha as compared to the UDF defined in the same post. When I limit the implementation here to alpha > 1 I get divergences and a different answer, unsurprisingly. The question then is, do we update pow or do I leave it for all alpha here?

@nhuurre
Copy link
Collaborator

nhuurre commented Sep 10, 2022

CDF derivative with respect to alpha is always zero. The derivatives with respect y and sigma are zero only if alpha > 1.
The derivative of LCDF is

$$ \frac{1}{e^{\left(\frac{y}{\sigma}\right)^{\alpha}}-1}\left(\frac{y}{\sigma}\right)^{\alpha}\log\left(\frac{y}{\sigma}\right)\approx \log\left(\frac{y}{\sigma}\right) $$

which goes to negative infinity at y=0—but realistically the only way to get a finite log-prob at the end is to put the LCDF value through a log_sum_exp/log_diff_exp or the equivalent and those are going to squash any finite derivative to zero. But unfortunately infinite derivatives probably still break even when they shouldn't. We could either just say the derivative here is zero (and trust that any situation where that leads to incorrect result is going to be rejected anyway due to a target=-inf issue or adjust log_sum_exp to ignore the derivatives of -inf terms.

@spinkney
Copy link
Collaborator Author

@rok-cesnovar I don't know how to update the opencl version of weibull cdf. It needs to return 0 for the gradients when y is 0 and I'm getting header check errors when I try to update.

@rok-cesnovar
Copy link
Member

Looking into it now.

@spinkney spinkney mentioned this pull request Oct 20, 2022
@andrjohns
Copy link
Collaborator

Obsoleted by #3037

@andrjohns andrjohns closed this Mar 24, 2024
@WardBrian WardBrian deleted the fix-weibull-lcdf branch April 12, 2024 18:10
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.

6 participants