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

feat: add Cruijff PDF #64

Merged
merged 7 commits into from
Mar 19, 2024
Merged

feat: add Cruijff PDF #64

merged 7 commits into from
Mar 19, 2024

Conversation

ikrommyd
Copy link
Contributor

@ikrommyd ikrommyd commented Mar 18, 2024

Adds Cruijff PDF as unnormalized density
Related to zfit/zfit#512

Proposed Changes

  • Add Cruijff PDF as unnormalized density

Tests added

  • tests/test_pdf_cruijff.py

Checklist

  • change approved
  • implementation finished
  • correct namespace imported
  • tests added
  • CHANGELOG updated

@ikrommyd
Copy link
Contributor Author

@jonas-eschle FYI this is ready for review.

Copy link
Contributor

@jonas-eschle jonas-eschle left a comment

Choose a reason for hiding this comment

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

Looks good overall, minor things, but other than that it's fine. A pity that no analytic integral is available, but fine, the numeric integrator catches it well. I'll approve, b

@ikrommyd
Copy link
Contributor Author

Looks good overall, minor things, but other than that it's fine. A pity that no analytic integral is available, but fine, the numeric integrator catches it well. I'll approve, b

I think there is just no anti-derivative. I tried with some integral calculators and got nowhere.

Copy link
Contributor

@jonas-eschle jonas-eschle left a comment

Choose a reason for hiding this comment

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

I've added the suggestion to refactor. I think it makes sense? If so, just apply the suggestion and the PR is approved

ikrommyd and others added 2 commits March 19, 2024 17:17
Co-authored-by: Jonas Eschle <jonas.eschle@cern.ch>
@ikrommyd
Copy link
Contributor Author

ikrommyd commented Mar 19, 2024

@jonas-eschle there was incorrect indentation in your suggestion but I fixed it. All good now, tests pass. Will merge as soon as ci completes.

@jonas-eschle
Copy link
Contributor

@jonas-eschle there was incorrect indentation in your suggestion but I fixed it. All good now, tests pass.

yep, just noticed, thanks for the fix

@ikrommyd ikrommyd merged commit 4f0bdb4 into zfit:develop Mar 19, 2024
12 checks passed
@ikrommyd ikrommyd deleted the feat-cruijff branch March 19, 2024 16:21
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