Skip to content

Upgrade to PineAPPL v1#206

Merged
scarlehoff merged 41 commits into
mainfrom
bump-pineappl-v1
Jun 11, 2025
Merged

Upgrade to PineAPPL v1#206
scarlehoff merged 41 commits into
mainfrom
bump-pineappl-v1

Conversation

@Radonirinaunimi
Copy link
Copy Markdown
Member

@Radonirinaunimi Radonirinaunimi commented Nov 13, 2024

This PR tests the new Python interface of the upcoming PineAPPL v1. There are a few notable changes affecting pineko directly:

  • The types of convolutions (be it for grids or FK tables) are accessed through the convolutions attributes. These information are no longer determined from the metadata (address the shortcomings discussed in Bump pineappl version nnpdf#2129 (comment) and Allow evolution and convolution with different ekos for a single grid  #181 (comment)).
  • Grids can be evolved with an arbitrary number of EKOs (no longer limited to two operators). This removes the need for operator1 and operator2 in favor of just operators.
  • Grids and FK tables can be convolved with an arbitrary number of PDFs (no longer limited to two PDFs). Similarly, this removes the need for pdf1 and pdf2 in favor of pdfs.
  • Add a new variation of scale - which is the fragmentation scale xia.

The only remaining thing for which we will still require an ad-hoc hack/tweak is the checks that the PDFs are ordered according to the type of convolutions. This is an LHAPDF problem so there isn't much that we can do.

This will deprecate #192.

Remaining to do:

  • profile the computation of the Jet EKOs

@Radonirinaunimi Radonirinaunimi added enhancement New feature or request physics Physics feature pineappl modifications related to PineAPPL labels Nov 13, 2024
@cschwan
Copy link
Copy Markdown
Contributor

cschwan commented Nov 14, 2024

When this is working I suggest you also try to implement #183 - this should decrease the time and space needed to perform an evolution. Also the required changes should be minimal.

@Radonirinaunimi
Copy link
Copy Markdown
Member Author

When this is working I suggest you also try to implement #183 - this should decrease the time and space needed to perform an evolution. Also the required changes should be minimal.

Thanks for letting me know about this! I will surely do once this is working indeed.

Comment thread .github/workflows/bench.yml Outdated
Copy link
Copy Markdown
Contributor

@giacomomagni giacomomagni left a comment

Choose a reason for hiding this comment

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

Looks great to me. Some really minor comments:

  • Just a small question: why key_values are still around and now we don't have just metadata ? Why the theory card is still saved to key_values ?

# propagate metadata
for k, v in ori_grid.key_values.items():
new_grid.set_key_value(k, v)

  • I did not check, I'm able to produce a real FKtable myself.

Comment thread src/pineko/check.py Outdated
Comment thread benchmarks/bench_comparator.py Outdated
Comment thread src/pineko/cli/convolve.py Outdated
Comment thread src/pineko/cli/fonll.py
Comment thread src/pineko/comparator.py Outdated
Comment thread src/pineko/evolve.py Outdated
Comment thread src/pineko/evolve.py Outdated
@cschwan
Copy link
Copy Markdown
Contributor

cschwan commented Feb 26, 2025

Looks great to me. Some really minor comments:

* Just a small question: why `key_values` are still around and now we don't have just metadata ? Why the theory card is still saved to `key_values` ?

Indeed, it should be {set_,}metadata instead of {set_,}key_value. I'm not why it still works the old way.

@Radonirinaunimi
Copy link
Copy Markdown
Member Author

Thanks a lot @giacomomagni for the review! I will address them once I am in front of a computer this afternoon.

Looks great to me. Some really minor comments:

* Just a small question: why `key_values` are still around and now we don't have just metadata ? Why the theory card is still saved to `key_values` ?

Indeed, it should be {set_,}metadata instead of {set_,}key_value. I'm not why it still works the old way.

Some of these set_key_values are redefinitions coming from FakeObject classes in the tests and hence why still work (and also why I didn't catch them). I will replace these when addressing the reviews.

Copy link
Copy Markdown
Contributor

@giacomomagni giacomomagni left a comment

Choose a reason for hiding this comment

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

As long as it works it's good for me!

Comment thread src/pineko/evolve.py Outdated
@Radonirinaunimi Radonirinaunimi removed the waiting-pineappl-v1 Waiting for the release of PineAPPL v1 label Feb 28, 2025
@Radonirinaunimi
Copy link
Copy Markdown
Member Author

As long as it works it's good for me!

Perfect, thanks! Let's wait for #215 and we can test it here.

@Radonirinaunimi Radonirinaunimi added the run-regression Trigger the regression test label Apr 8, 2025
@Radonirinaunimi
Copy link
Copy Markdown
Member Author

Everything here is now done! The only thing missing here is the stable version of PineAPPL; as soon as that is available we can bump the version here and merge.

Comment thread src/pineko/check.py Outdated
Comment thread src/pineko/comparator.py Outdated
Copy link
Copy Markdown
Member

@scarlehoff scarlehoff left a comment

Choose a reason for hiding this comment

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

I'm currently using this branch and seems to work well :). The only problem I've found up to now is what I already reported when there are empty bins.

Comment thread src/pineko/cli/compare.py Outdated
@scarlehoff
Copy link
Copy Markdown
Member

Hi @Radonirinaunimi one request before merging this (and I think it is better to do it here since it changes in 1.0), could we ensure that all metadata from the original grid is propagated to the fktable?

@Radonirinaunimi
Copy link
Copy Markdown
Member Author

Hi @Radonirinaunimi one request before merging this (and I think it is better to do it here since it changes in 1.0), could we ensure that all metadata from the original grid is propagated to the fktable?

Yes! In principle this should be the case but I will explicitly double-check.

@scarlehoff
Copy link
Copy Markdown
Member

Great!

(just to say, I didn't check it wasn't the case, I was looking at the changes and realized that in the scale_variations.py the propagation is explicit but I couldn't find in the evolve, but perhaps I just missed it, too many changes)

@Radonirinaunimi
Copy link
Copy Markdown
Member Author

Radonirinaunimi commented Jun 2, 2025

Hi @Radonirinaunimi one request before merging this (and I think it is better to do it here since it changes in 1.0), could we ensure that all metadata from the original grid is propagated to the fktable?

Sorry, I missed to reply to this earlier. The metadata are always automatically propagated into the FK tables. Scale varied grids/FK tables are created from new instances so that's why it is needed to manually copy the metadata.

@Radonirinaunimi Radonirinaunimi added run-regression Trigger the regression test and removed run-regression Trigger the regression test labels Jun 10, 2025
@Radonirinaunimi Radonirinaunimi added run-regression Trigger the regression test and removed run-regression Trigger the regression test labels Jun 11, 2025
@scarlehoff scarlehoff merged commit 815bdff into main Jun 11, 2025
7 checks passed
Copy link
Copy Markdown
Contributor

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

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

I didn't read the big evolve.py and theory.py (yet), but I found a stupid typo

Comment thread src/pineko/cli/convolve.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request pineappl modifications related to PineAPPL run-regression Trigger the regression test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants