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

Use the NNPDF data instead of yamldb #159

Merged
merged 14 commits into from
Mar 26, 2024
Merged

Use the NNPDF data instead of yamldb #159

merged 14 commits into from
Mar 26, 2024

Conversation

scarlehoff
Copy link
Member

@scarlehoff scarlehoff commented Mar 14, 2024

With this pineko will use nnpdf whenever the nnpdf=True key is set in the [general] part of the config.

Since when using nnpdf, the yamldb is not needed, I've dropped it from required keys.

At the moment this is not the default. I think it should become default because the NNPDF no longer looks at the yamldb files, the data is the only source of truth, so the fktables should have the same names. But this is something that can always be done manually afterwards.

@scarlehoff scarlehoff requested a review from andreab1997 March 14, 2024 14:31
@felixhekhorn felixhekhorn added the enhancement New feature or request label Mar 14, 2024
Copy link
Contributor

@andreab1997 andreab1997 left a comment

Choose a reason for hiding this comment

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

Ok I see and it is fine for me.

I see that this is a proof of concept, but are you planning to do the full job here? In other words, should I take care of this?

@scarlehoff
Copy link
Member Author

Nono, I will. I just wanted to know whether you were ok with the approach.

I hope to merge tomorrow this NNPDF/nnpdf#1965 so I will finish it when that's in.

@giacomomagni giacomomagni linked an issue Mar 15, 2024 that may be closed by this pull request
@giacomomagni
Copy link
Contributor

This would have the advantage of being able to read the kinematics from there,
instead from the grid.

@scarlehoff
Copy link
Member Author

This is ready for review (and modifications) @andreab1997

At the moment is using the whole of validphys since it uses the reader. It could only use the data through NNPDF/nnpdf#1965 but since at the moment it is optional I think it is ok.
If you want to make it default you need to load the theory part of the metadata yourself from here (or move the parser to the data package in validphys).

I have not updated the dependencies because at the moment they are quire restrictive and don't want to go around updating eko/banana/python/etc. I've tried putting 3.9-3.13 and that broke banana.

I've updated only pineappl (which should be updated also in master).

@alecandido
Copy link
Member

I have not updated the dependencies because at the moment they are quire restrictive and don't want to go around updating eko/banana/python/etc. I've tried putting 3.9-3.13 and that broke banana.

3.13 is definitely unsupported :P

@scarlehoff
Copy link
Member Author

It's <3.13

@felixhekhorn
Copy link
Contributor

felixhekhorn commented Mar 18, 2024

I've tried putting 3.9-3.13 and that broke banana.

banana is 3.12 ready: https://github.com/N3PDF/banana/blob/fc46349d8c0abd778afe0d2f949be2ec3f3a6bda/pyproject.toml#L28 (as we needed it for eko) - the blocker might be pineko itself (also yadism didn't catch up yet, but that has no part here)

edit: blub you're trying to fix pineko 🙈

@andreab1997
Copy link
Contributor

Details

Ok I will review this ASAP

Copy link
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.

@scarlehoff Right now the yamldb is used also in the kfactor module, can you change that one as well ?

@scarlehoff
Copy link
Member Author

Do you want me to do it before #161 ?

The changes needed for this are very minimal so it might be better to wait?

@scarlehoff
Copy link
Member Author

on second thoughts, should this even touch the kfactors part? We should not need kfactors going forward, specially for the new data?

@felixhekhorn
Copy link
Contributor

Do you want me to do it before #161 ?

The changes needed for this are very minimal so it might be better to wait?

I think whoever is ready to be merged first can go ... we need to sort out the dependency problem here first: at the moment Github is just complaining that the lock file is outdated

on second thoughts, should this even touch the kfactors part? We should not need kfactors going forward, specially for the new data?

in the long run maybe yes, but surely we still need support for a while (e.g. for EW)

src/pineko/theory.py Outdated Show resolved Hide resolved
@scarlehoff
Copy link
Member Author

in the long run maybe yes, but surely we still need support for a while (e.g. for EW)

Not from pineko

@alecandido
Copy link
Member

in the long run maybe yes, but surely we still need support for a while (e.g. for EW)

I believe there are two point of views:

  • Pineko (or whoever else) will burn kfactors in the grids, so downstream you shouldn't need kfactors any longer
  • the kfactors themselves you will need forever: there will always be a calculation for which you don't have a generator, or you only have results that are not sufficiently exclusive - and there will be someone willing to include them anyhow, because a kfactor is a better approximation than nothing

@scarlehoff
Copy link
Member Author

Why is pylint being applied to the benchmark as well?

@scarlehoff
Copy link
Member Author

Ok, this is done as far as I'm concerned. Hopefully it even works. I've been using it for the perturbative charm theories.

The kfactor at the moment (current main) has the yamldb_path as a command line argument which is passed through (while this PR instead overrides the yamldb from the configuration).

I can modify if you want but that is just going to create unnecessary conflicts with #161.

@andreab1997 andreab1997 self-requested a review March 19, 2024 11:06
@felixhekhorn
Copy link
Contributor

Why is pylint being applied to the benchmark as well?

mh? it is not:

lint = "pylint src/ -E"

problem is pylint needs to be update to be compatible with 3.12 - see also NNPDF/eko#349

Copy link
Contributor

@andreab1997 andreab1997 left a comment

Choose a reason for hiding this comment

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

Other than the trivial comment about the function location, this is ok for me

src/pineko/theory.py Outdated Show resolved Hide resolved
@scarlehoff
Copy link
Member Author

Why is pylint being applied to the benchmark as well?

mh? it is not:

lint = "pylint src/ -E"

problem is pylint needs to be update to be compatible with 3.12 - see also NNPDF/eko#349

Not the benchmark folder but rather in the benchmark task.

@felixhekhorn
Copy link
Contributor

Not the benchmark folder but rather in the benchmark task.

I see - boh' the code should be just always checked and it doesn't hurt (and in case it does we know immediately that something is fishy - even if it is just the pylint version)

@scarlehoff
Copy link
Member Author

But repeating the same check in several different workflows just reduces the amount of information you get out of the failures.

@alecandido
Copy link
Member

But repeating the same check in several different workflows just reduces the amount of information you get out of the failures.

Actually, I wanted to have a separate workflow called analysis to just run Pylint and Mypy (but we never got to that point). You're free to contribute it, it will be very simple :)

@scarlehoff
Copy link
Member Author

scarlehoff commented Mar 21, 2024

Before merging this I would like to prepare a test so please don't yet.

edit: you cannot merge it if I forget to add the files. Genius!

Copy link
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.

Some minor comments:

  • Please update the docs here, so we keep track of the changes.
  • This line has to be removed.

src/pineko/configs.py Show resolved Hide resolved
@scarlehoff
Copy link
Member Author

This line has to be removed.

Why? This is not using the theories from NNPDF.

@giacomomagni
Copy link
Contributor

Why? This is not using the theories from NNPDF.

Sure, but it doesn't use the yamldb as well, it only looks at theory cards.

@scarlehoff scarlehoff merged commit add5493 into main Mar 26, 2024
6 checks passed
@scarlehoff scarlehoff deleted the use_nnpdf_data branch March 26, 2024 14:13
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.

Drop yamldb as default
5 participants