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

support Lgamma and BroadcastTo Ops in order to support probabilistic models (Poisson distribution) #2011

Closed
lukasheinrich opened this issue Sep 10, 2019 · 13 comments

Comments

@lukasheinrich
Copy link

lukasheinrich commented Sep 10, 2019

When trying to covnert a model that uses tensorflow_probability.distributions.Poisson I get the error

ValueError: Unsupported Ops in the model before optimization
Lgamma, BroadcastTo

In order to support probabilistic models using those libraries itwould be great if those Ops could be added

@kratsg @matthewfeickert

https://www.tensorflow.org/api_docs/cc/class/tensorflow/ops/lgamma
https://www.tensorflow.org/api_docs/cc/class/tensorflow/ops/broadcast-to

@dsmilkov
Copy link
Contributor

dsmilkov commented Sep 18, 2019

Hi Lukas,

Can you share a bit about your model/use-case? There's a long tail of ops that TF.js is missing and we are trying to prioritize based on use-case.

@lukasheinrich
Copy link
Author

lukasheinrich commented Sep 26, 2019

hi @dsmilkov,

our specific use case is the computation of probabilistic models for high energy physics models at CERN, using this package:

https://github.com/diana-hep/pyhf

but I think being able to compute a Poisson distribution is probably of much broader appeal for any model using tensorflow probability.

@DirkToewe
Copy link
Contributor

The rather monstrous PR#1356 contained a broadcastTo implementation. If You want, I can take it out and submit it as PR to the monorepo.

@lukasheinrich
Copy link
Author

hi @DirkToewe I think that would be awesome and maybe easier to merge. Do you know anytone working on lgamma?

@DirkToewe
Copy link
Contributor

DirkToewe commented Oct 19, 2019

No idea sorry. A float32/complex64 implementation of lgamma(z) for Re(z) > 0 can be found in:

"Numerical Recipes in C",
2nd edition,
chapter 6, page 213f.

It's C code but fairly easy to read and understand. Should be feasible to implement using existing TFJS functions as well.

@kratsg
Copy link

kratsg commented Oct 20, 2019

@DirkToewe -- copying here for usefulness

#include <math.h>

// Returns the value ln[Γ(xx)] for xx > 0.
float gammln(float xx)
{
//Internal arithmetic will be done in double precision, a nicety that you can omit if five-figure
// accuracy is good enough.
    double x,y,tmp,ser;
    static double cof[6]={76.18009172947146,-86.50532032941677,
                          24.01409824083091,-1.231739572450155,
                          0.1208650973866179e-2,-0.5395239384953e-5};
    int j;
    y=x=xx;
    tmp=x+5.5;
    tmp -= (x+0.5)*log(tmp);
    ser=1.000000000190015;
    for (j=0;j<=5;j++) ser += cof[j]/++y;
    return -tmp+log(2.5066282746310005*ser/x);
}

@annxingyuan
Copy link
Contributor

Noting that BroadcastTo is also needed for the music transformer model.

@gaikwadrahul8
Copy link
Contributor

gaikwadrahul8 commented May 1, 2023

Hi, @lukasheinrich

Apologize for the delayed response and I see this PR #2238 got merged for tf.broadcastTo but at the moment It seems like we haven't implemented Lgamma Op so just to confirm still are you looking to add this Lgamma Op please ?

If someone wants to contribute for Lgamma Op then you're always welcome please feel free to do that, please refer this link. Thank you!

@google-ml-butler
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you.

@kratsg
Copy link

kratsg commented May 11, 2023

Apologize for the delayed response and I see this PR #2238 got merged for tf.broadcastTo but at the moment It seems like we haven't implemented Lgamma Op so just to confirm still are you looking to add this Lgamma Op please ?

We definitely would want the LGamma op if you want to support Poisson distributions fully, no?

@google-ml-butler
Copy link

Closing as stale. Please @mention us if this needs more attention.

@kratsg
Copy link

kratsg commented May 18, 2023

@google-ml-butler not stale...

@matthewfeickert
Copy link

@dsmilkov as the @google-ml-butler isn't taggable on GitHub can you unstale this?

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

No branches or pull requests

8 participants