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

NNLOJET runner #65

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

NNLOJET runner #65

wants to merge 16 commits into from

Conversation

scarlehoff
Copy link
Member

@scarlehoff scarlehoff commented Mar 1, 2024

This is very much work in progress.

For now I've added:

  1. Autogeneration of a pinecard given an NNPDF dataset (using the new reader).
pinefarm autogen LHCB_Z0_7TEV_MUON_Y

Still todo:

  • separate the metadata information in metadata.txt and possibly come up with a better name for the runcards
  1. Autogeneration of NNLOJET runcards at run-time.
pinefarm run NNLOJET_LHCB_Z0_7TEV_MUON_Y theories/200.yaml

This command autogenerates the runcards and tries to run them... if you have NNLOJET installed. Otherwise it will just exit and tell you where the runcards are.

I think this is the minimal think we should require everyone running Matrix. At least to get to the point where the code is installed and the runcards prepared.

Still todo:

  • Use the theory runcard to fill the theory parameters. At the moment they are defaults that I set in the module.
  1. ????

  2. profit: (still not pushed, but done) autogeneration of results and pineappl grids from the results product

You'll notice that 3. is missing, that's where the difficulty lies (and the part that will be shared with Matrix anyway). At the moment pinefarm hopes to be able to run the program by just doing run <dataset> <theory>.

It will be necessary to add at least two sets of options: --warmup/production (and then --cores, --iterations, --events, --jobs).
And --channel which would allow the user to run LO, NNLO, RR, RR_collinear_region etc, regardless of the PTO.
I also have a rough draft for that but at the moment is NNLOJET-only, it would be good to do it in a way that can be easily used by other tools.

Note:
I'll revert some of the changes to pre-commit / pyproject when I'm done, pinefarm should be much more slim in terms of dependencies so I'll see how I fix that as well, I'll separate those commits.
Same for the ignoring and the warning and the autogeneration of theories. I do think someone installing the code should just be able to run it with some minimal default configuration, I think it might motivate people to actually use it ("oh, so cool, I just have to do pip install pinefarm and I can create a grid with no errors or warnings!!")

@scarlehoff scarlehoff marked this pull request as draft March 1, 2024 18:03
src/pinefarm/info.py Outdated Show resolved Hide resolved
@scarlehoff scarlehoff force-pushed the nnlojet_interface_template branch 2 times, most recently from 54e1fdd to 23ec6ac Compare March 5, 2024 16:13
@felixhekhorn felixhekhorn added the enhancement New feature or request label Mar 6, 2024
@scarlehoff scarlehoff force-pushed the nnlojet_interface_template branch 4 times, most recently from 22750f2 to 034dcad Compare March 22, 2024 10:36
@scarlehoff scarlehoff changed the base branch from main to hide_away_imports March 22, 2024 10:36
Base automatically changed from hide_away_imports to main March 22, 2024 14:13
@scarlehoff scarlehoff force-pushed the nnlojet_interface_template branch from 91a5706 to 5f559dc Compare March 22, 2024 14:15
Comment on lines +318 to +323
if process.startswith("WPWM"):
processes = [process.replace("WP", ""), process.replace("WM", "")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if process.startswith("WPWM"):
processes = [process.replace("WP", ""), process.replace("WM", "")]

Removing these two lines seems to solve the problem of the right ds generation for CMS_WPWM_13TEV_ETA_WM and CMS_WPWM_13TEV_ETA_WP.
Not sure, however, whether it might break something else somewhere else where this split is needed instead

Copy link
Member Author

Choose a reason for hiding this comment

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

It will for all other datasets (yours are the first for which the split is done at the level of the dataset).

It needs to be modified to look at the fktables to see whether they include both (probably a len is enough for now though)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, maybe it does not really solve it as proc is now WPWM.

I think we might have to adopt a convention for the WPWM datasets.

In the dataset CMS_WPWM_13TEV_ETA the two observables, WP and WM, are separated, hence splitting as it is done in line 319 above makes sense there. However, this is not the case for other datasets.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be fair, Wp ans Wm are separated everywhere. They are just concatenated.

The questions are, do we want to keep WPWM as the only type of dataset (since the data for Wp and Wm usually come together, I'd vote yes)

In that case, how do we distinguish.
I think having the observable as WP and WM as you did is a good way if we are consistent

(and then this if can stay BUT when the observable is WM it only generates WM)

@scarlehoff scarlehoff force-pushed the nnlojet_interface_template branch from 5b5b8ac to ca82c2e Compare November 4, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants