feat: add `Hypatia2` pdf #84

qishi-hep · 2024-09-18T05:54:48Z

Add Hypatia2 pdf, two-sided version of the Hypatia distribution described in https://arxiv.org/abs/1312.5000, based on this snippet

Proposed Changes

added zfit_physics/models/pdf_hypatia2.py

Tests added

added tests/test_pdf_hypatia2.py

Checklist

ikrommyd · 2024-09-20T05:23:21Z

Hi @qishi-hep. Thanks for making this.
I haven't looked at the code yet to review but I have two small requests at first sight.

Please rename Ipatia -> Hypatia for consistency with ROOT and also because that's how it's spelled properly.
It would be nice if you add some testing against ROOT. It won't be tested in CI but the tests can run locally and it's good to have them for future reference. Look at https://github.com/zfit/zfit-physics/blob/develop/tests/test_pdf_novosibirsk.py where this is happening for the Novosibirsk distribution. Tests can be skipped in CI by doing ROOT = pytest.importorskip("ROOT")

qishi-hep · 2024-09-23T08:58:11Z

Hi @ikrommyd, I modified the code according to your comments. The test again RooFit passed locally. You can have a look :)

jonas-eschle · 2024-09-24T13:57:21Z

zfit_physics/models/pdf_hypatia2.py

+    second_cond = tf.logical_and(tf.logical_and(x < 1.0e-04, nu > 0.0), nu < 55)
+    third_cond = tf.logical_and(x < 0.1, nu >= 55)
+    cond = tf.logical_or(first_cond, tf.logical_or(second_cond, third_cond))
+    return znp.where(cond, low_x_BK(nu, x), bessel_kv_func(nu, x))


Slightly concerned about this one: Will the wrong numbers cause issues anywhere here, i.e. Nans in the bessel_kv_func?

The point is that both branches are evaluated, and while it's fine for the normal where, unfortunately the gradient can be affected by this (yep, annoying), as it's calculated by multiplying the not-used branch by 0. But this results again in NaNs.

Can we check the gradients of the function too? @ikrommyd we did that already in the past, didn't we? I just could not find the pdf, was it zfit?

We didn't check gradients in ci. But I did that manually for the Faddeeva function. For PDFs so far we just knew that we weren't doing any operations that may cause this.

Ah okay! Probably worth to add a check then

Yeah, it looks like there is a problem.

Great, thanks a lot for checking! Okay, yep, I guess we need again "safe values", do you remember how that works? We need to first create an array of arguments where the unused arguments, the ones that should not be taken and evaluate to NaNs are actually set to something harmless that does not produce anything wrong (i.e. a small, constant value). The value we use doesn't matter as we won't use it later on, since we use the where, but it's just not to screw up the gradient (i.e. the boolean to decide what goes where is gonna be used two times (or more)

There is the safe_where function in zfit.z as well no?

I added safe wheres but it doesn't fix the problem. Gradients are still nan. See 3b95ae2

Maybe there is another problem somewhere else. I'll try to check again tomorrow.

@qishi-hep There are some issues with the gradients. They start from tfp probably because the Bessel kve doesn't have a gradient with respect to one parameter. There may probably be places elsewhere that make the gradiends go nan. We'll debug and see what we can do about it.

jonas-eschle · 2024-09-24T13:57:46Z

Hi @qishi-hep thanks a lot for the PR and welcome to the contributors! It looks overall quite good to me.

ikrommyd · 2024-09-24T16:37:22Z

Looks good to me as well. I will add a little bit more testing and maybe a gradiends check.
What I'd also like to do is change the order of the arguments in the main class to match the root version of the docstring. I don't like what root is doing where they name the "left" components without a number and the "right" components they just add a "2".

jonas-eschle · 2024-09-24T18:18:02Z

What I'd also like to do is change the order of the arguments in the main class to match the root version of the docstring. I don't like what root is doing where they name the "left" components without a number and the "right" components they just add a "2".

I think the main argument would be to be consintent with the CBs that we have, but that agrees with RooFits. The only inconsistency I se now is that we call it so fal alphal and not al, opinions? Consistency would be nice, RooFit calls it alphal, but al also seems neat to me? (because the alphas are "the same", the Hypatia just "adds" stuff to the DCB, AFAIU)

ikrommyd · 2024-09-24T18:46:43Z

What I'd also like to do is change the order of the arguments in the main class to match the root version of the docstring. I don't like what root is doing where they name the "left" components without a number and the "right" components they just add a "2".

I think the main argument would be to be consintent with the CBs that we have, but that agrees with RooFits. The only inconsistency I se now is that we call it so fal alphal and not al, opinions? Consistency would be nice, RooFit calls it alphal, but al also seems neat to me? (because the alphas are "the same", the Hypatia just "adds" stuff to the DCB, AFAIU)

Hmm, yeah I did the change to be consistent with the paper and add more tests, I can change a->alpha sure.

jonas-eschle · 2024-09-24T21:54:08Z

Hmm, yeah I did the change to be consistent with the paper and add more tests, I can change a->alpha sure.

... we can also change the other way around, all alphal -> al in the other PDFs, no strong feelings about it (but consistency could be nice, it seems like RooFit is inconsistetn here with CrystalBall vs Hypathia2)

qishi-hep · 2024-09-25T07:59:15Z

Thanks for the great changes 👍. For variable name, I think alphal is better, changing alphal to al in other PDFs can break a lot of existing code.

jonas-eschle · 2024-11-01T19:32:23Z

Hi @qishi-hep and @ikrommyd , I've added a feature in zfit to specify the parameters that support automatic gradients (by default all). If I remember correctly @ikrommyd , it was lambda that wasn't diffable?

I've force pushed and rebased, as I've restructured the repos a bit. Make sure to first pull!
I think the stopping items have therefore been removed, so we can continue on the PR

ikrommyd · 2024-11-01T19:47:11Z

Hi @qishi-hep and @ikrommyd , I've added a feature in zfit to specify the parameters that support automatic gradients (by default all). If I remember correctly @ikrommyd , it was lambda that wasn't diffable?

I've force pushed and rebased, as I've restructured the repos a bit. Make sure to first pull! I think the stopping items have therefore been removed, so we can continue on the PR

It's the first argument of bessel_kve that is not diffable according to: https://www.tensorflow.org/probability/api_docs/python/tfp/math/bessel_kve

jonas-eschle · 2024-11-01T20:16:02Z

Yes, I remember the same and that it was'nt realistically possible to get it right, and that's lambda propagated, afai can see

for more information, see https://pre-commit.ci

…ambda

qishi-hep changed the title ~~Add Ipatia2 pdf~~ feat: add Ipatia2 pdf Sep 18, 2024

qishi-hep changed the title ~~feat: add Ipatia2 pdf~~ feat: add Ipatia2 pdf Sep 18, 2024

qishi-hep changed the title ~~feat: add Ipatia2 pdf~~ feat: add Hypatia2 pdf Sep 23, 2024

jonas-eschle reviewed Sep 24, 2024

View reviewed changes

jonas-eschle force-pushed the qshi/Ipatia2 branch 2 times, most recently from 10bba2e to 99684eb Compare November 1, 2024 19:25

jonas-eschle force-pushed the qshi/Ipatia2 branch from 7bdb6cc to 25c0ee3 Compare November 1, 2024 19:40

Qi Shi and others added 12 commits November 1, 2024 16:19

Add Ipatia2 pdf

cbf84cc

[pre-commit.ci] auto fixes from pre-commit.com hooks

2f87262

for more information, see https://pre-commit.ci

add CHANGELOG & rename "l" to "lam"

6c891af

[pre-commit.ci] auto fixes from pre-commit.com hooks

21e6ac3

for more information, see https://pre-commit.ci

update docstring

2ffdb01

Rename ipatia2 to hypatia2

2a6b75f

add comparison to RooFit

08a118e

variable names and order changes + more testing

0d8318f

rename a->alpha to be consistent with crystal balls

8c76437

fix backslashes in docstring

42e46a1

add safe wheres

5e0fbb8

enh: use the autograd_params to not take a gradient with respect to l…

6a9e176

…ambda

fix: typing

ee0bec5

jonas-eschle force-pushed the qshi/Ipatia2 branch from 25c0ee3 to ee0bec5 Compare November 1, 2024 20:19

Merge branch 'develop' into qshi/Ipatia2

f1dc60d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `Hypatia2` pdf #84

feat: add `Hypatia2` pdf #84

qishi-hep commented Sep 18, 2024 •

edited

Loading

ikrommyd commented Sep 20, 2024 •

edited

Loading

qishi-hep commented Sep 23, 2024

jonas-eschle Sep 24, 2024

ikrommyd Sep 24, 2024

jonas-eschle Sep 24, 2024

ikrommyd Sep 24, 2024

jonas-eschle Sep 24, 2024

ikrommyd Sep 24, 2024

ikrommyd Sep 24, 2024

ikrommyd Sep 24, 2024

ikrommyd Sep 25, 2024

jonas-eschle commented Sep 24, 2024

ikrommyd commented Sep 24, 2024 •

edited

Loading

jonas-eschle commented Sep 24, 2024

ikrommyd commented Sep 24, 2024

jonas-eschle commented Sep 24, 2024

qishi-hep commented Sep 25, 2024

jonas-eschle commented Nov 1, 2024

ikrommyd commented Nov 1, 2024

jonas-eschle commented Nov 1, 2024

feat: add Hypatia2 pdf #84

Are you sure you want to change the base?

feat: add Hypatia2 pdf #84

Conversation

qishi-hep commented Sep 18, 2024 • edited Loading

Proposed Changes

Tests added

Checklist

ikrommyd commented Sep 20, 2024 • edited Loading

qishi-hep commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-eschle commented Sep 24, 2024

ikrommyd commented Sep 24, 2024 • edited Loading

jonas-eschle commented Sep 24, 2024

ikrommyd commented Sep 24, 2024

jonas-eschle commented Sep 24, 2024

qishi-hep commented Sep 25, 2024

jonas-eschle commented Nov 1, 2024

ikrommyd commented Nov 1, 2024

jonas-eschle commented Nov 1, 2024

feat: add `Hypatia2` pdf #84

feat: add `Hypatia2` pdf #84

qishi-hep commented Sep 18, 2024 •

edited

Loading

ikrommyd commented Sep 20, 2024 •

edited

Loading

ikrommyd commented Sep 24, 2024 •

edited

Loading