Skip to content

Update the calls to axes to evolve_info#84

Merged
scarlehoff merged 6 commits into
mainfrom
remove_axes_call
Mar 31, 2023
Merged

Update the calls to axes to evolve_info#84
scarlehoff merged 6 commits into
mainfrom
remove_axes_call

Conversation

@scarlehoff
Copy link
Copy Markdown
Member

Closes #72

If I understood evolve_info correctly this should be doing the same as was done before. The only thing that I'm not sure about is what is the implicit masking that axes was doing? Or maybe axes assumed that all orders had the exact same x/q structure?

Comment thread src/pineko/evolve.py Outdated
@cschwan
Copy link
Copy Markdown
Contributor

cschwan commented Mar 24, 2023

@scarlehoff the old behaviour is to use all orders (mask set to all true), which potentially makes the EKOs larger than needed. That's not wrong just less performant.

Copy link
Copy Markdown
Contributor

@cschwan cschwan left a comment

Choose a reason for hiding this comment

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

Superficially this looks good to me. However, I suppose you to test it if you haven't done it to make sure the order treatment is correct.

@scarlehoff
Copy link
Copy Markdown
Member Author

For now I've tested that it works with merged grids. I'll test it with a random fktables from the NLO and NNLO theories that @andreab1997 has in dom.

Comment thread src/pineko/evolve.py
@andreab1997
Copy link
Copy Markdown
Contributor

Seems good even if in #75 is done in a different way

Copy link
Copy Markdown
Collaborator

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

Do not ask for a maximum value, just create a trivial mask

Comment thread src/pineko/check.py
Copy link
Copy Markdown
Collaborator

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

Improvements (i.e. simplifications) postponed to #85. The rest looks perfect to me :)

@felixhekhorn felixhekhorn added the refactor Refactor code label Mar 24, 2023
@scarlehoff
Copy link
Copy Markdown
Member Author

In order to test this I'm redoing all fktables in theory 400.

DIS agree perfectly with what @andreab1997 has in dom for theories 4054 (NLO) and 4244 (NNLO).
ATLAS FKTables have already finished and they also agree*. CMS tables are under construction.

However, I've found a bug for ATLASDY2D8TEV. I don't think it is introduced by this PR but rather a bug in evolve_info (or in the combination of evolve_info, evolve and grids converted from applgrid) so I would wait until this is understood to merge this PR. I've written about the bug here: #72 (comment)

*btw, a bit more than 24 hours to finish all ATLAS FkTable -no jets- using a single core in the eko template.

@andreab1997
Copy link
Copy Markdown
Contributor

In order to test this I'm redoing all fktables in theory 400.

DIS agree perfectly with what @andreab1997 has in dom for theories 4054 (NLO) and 4244 (NNLO). ATLAS FKTables have already finished and they also agree*. CMS tables are under construction.

However, I've found a bug for ATLASDY2D8TEV. I don't think it is introduced by this PR but rather a bug in evolve_info (or in the combination of evolve_info, evolve and grids converted from applgrid) so I would wait until this is understood to merge this PR. I've written about the bug here: #72 (comment)

*btw, a bit more than 24 hours to finish all ATLAS FkTable -no jets- using a single core in the eko template.

What is the status of this? Can this be merged?

@scarlehoff
Copy link
Copy Markdown
Member Author

We need a fix (I believe in the pineappl side) for the situation in which evol_info doesn't return the actual grid used during the operation (which axes did) otherwise you won't be able to generate theories with evol_info (in principle also #75 should be broken for the same reason).

@cschwan believes is a numerical precision problem within pineappl

@scarlehoff
Copy link
Copy Markdown
Member Author

Ok, this is ready. I've updated pyproject.toml since this needs pineappl > 0.6.0 alpha 5

@alecandido
Copy link
Copy Markdown
Collaborator

alecandido commented Mar 31, 2023

@scarlehoff can you run poetry lock and upload the updated lock file?

This is what is currently blocking the tests.

@scarlehoff
Copy link
Copy Markdown
Member Author

Yes... but poetry seems to be having a really hard time resolving the dependencies... (it's about to do 10 minutes...) (I had installed pineappl from the repo so forgot about the lockfile)

@alecandido
Copy link
Copy Markdown
Collaborator

alecandido commented Mar 31, 2023

Are you using Poetry 1.4.1? They recently improved a lot the performance (recently = months).

@alecandido
Copy link
Copy Markdown
Collaborator

❯ poetry lock
Updating dependencies
Resolving dependencies... (7.1s)

Writing lock file

@alecandido
Copy link
Copy Markdown
Collaborator

@scarlehoff I'd be in favor of merging :)

@scarlehoff
Copy link
Copy Markdown
Member Author

❯ poetry lock
Updating dependencies
Resolving dependencies... (7.1s)

Writing lock file

Thanks. I'm at 2490s now... And no, I have poetry 1.2.2. Time to update I guess.

@scarlehoff scarlehoff merged commit f8c3261 into main Mar 31, 2023
@scarlehoff scarlehoff deleted the remove_axes_call branch March 31, 2023 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace Grid::axes with Grid::evolve_info

5 participants