Skip to content

Fix info file of mc2hessian#1551

Merged
scarlehoff merged 3 commits into
masterfrom
hotfix_mc2hessian
Apr 8, 2022
Merged

Fix info file of mc2hessian#1551
scarlehoff merged 3 commits into
masterfrom
hotfix_mc2hessian

Conversation

@scarlehoff
Copy link
Copy Markdown
Member

@scarlehoff scarlehoff commented Apr 8, 2022

I'll add also a test for this.

Fixes #1550

@scarlehoff scarlehoff requested a review from Zaharid April 8, 2022 10:18
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 8, 2022

Could we use a YAML parser for this? The silly search and replace thing was done because bootstrapping YAML there was difficult in 2015 for some reason.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 8, 2022

The test is great but I have a minor quibble with it: The environment variable prepends the new path but does not disable the old ones https://gitlab.com/hepcedar/lhapdf/-/blob/513b44b3d6864acd805bdefc7caab6414be5371c/src/Paths.cc#L30
so there is the theoretical possibility that something gets shadowed with the installed PDFs. I would suggest doing this instead:

which could be wrapped as something like

import contextlib

@contextlib.contextmanager
def temp_lhapdf_path(folder):
    oldpaths = lhapdf.paths()
    lhapdf.setPaths([str(folder)])
    try:
        yield
    finally:
        lhapdf.setPaths(oldpaths)

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Apr 8, 2022

Could we use a YAML parser for this? The silly search and replace thing was done because bootstrapping YAML there was difficult in 2015 for some reason.

Turns out it is still difficult in 2022 because LHAPDF doesn't really use standard YAML: https://github.com/N3PDF/eko/blob/341e6323e680d2320ae4400da61c92a445160416/src/ekobox/genpdf/export.py#L88

RE the test, fine, I'll add that.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 8, 2022

Why is that? They do use a yaml parser:

https://gitlab.com/hepcedar/lhapdf/-/tree/main/src/yamlcpp

@scarlehoff
Copy link
Copy Markdown
Member Author

I'll direct you to the people who know why that was needed @alecandido @andreab1997

That said, it might be that's the case now but not historically and so past lhapdf are not truly yaml.

Then there's the question of supporting old codes (fortran, c) that might rely on the exact format of lhapdf info files (like the order, lists being enclosed in [] etc).

@alecandido
Copy link
Copy Markdown
Contributor

Unfortunately I've not done it directly, it was really @andreab1997. I still hope I could do as much as possible without installing lhapdf, but in the simplest possible way (otherwise I could install lhapdf). And, unfortunately, when @andreab1997 attempted to parse info files it turned out that it was not standard YAML.

At this point, I wonder if it's just an old standard YAML, since lhapdf is vendoring the YAML library, and most likely not updating it very frequently...

@alecandido
Copy link
Copy Markdown
Contributor

I know that YAML is wonderful for readability, but I'm more and more tempted to step on the JSON side: the full YAML spec is damn complicated 😞

The best compromise I'm aware of is https://hitchdev.com/strictyaml/

@scarlehoff
Copy link
Copy Markdown
Member Author

attempted to parse info files it turned out that it was not standard YAML.

We've been always parsing the lhapdf info files using yaml

that was last changed 4 years ago. Maybe it was a problem with some specific pdf sets? I remember there were sets in lhapdf that were broken but that's hardly our fault.

I know that YAML is wonderful for readability, but I'm more and more tempted to step on the JSON side: the full YAML spec is damn complicated

It's not up to us here though.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 8, 2022

Are there any example of correct commonly used LHAPDF sets (i.e. that are in the official repository) that cannot be parsed with yaml and yet work with lhapdf proper?

If not it seems to me we should proceed, as indeed we are doing elsewhere.

@andreab1997
Copy link
Copy Markdown
Contributor

I'll direct you to the people who know why that was needed @alecandido @andreab1997

That said, it might be that's the case now but not historically and so past lhapdf are not truly yaml.

Then there's the question of supporting old codes (fortran, c) that might rely on the exact format of lhapdf info files (like the order, lists being enclosed in [] etc).

Are there any example of correct commonly used LHAPDF sets (i.e. that are in the official repository) that cannot be parsed with yaml and yet work with lhapdf proper?

If not it seems to me we should proceed, as indeed we are doing elsewhere.

I remember that I had this problem with CT14 probably. I don't remember exactly the PDF set but I can check. Anyway I believe that this problem is only in some old PDF sets.

@alecandido
Copy link
Copy Markdown
Contributor

I remember that I had this problem with CT14 probably. I don't remember exactly the PDF set but I can check. Anyway I believe that this problem is only in some old PDF sets.

This supports the conjecture of an oldish YAML library: maybe they are edge cases that could be parsed by every library at that time, but then spec and libraries evolved, and now some old files are violating something in the new specs.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 8, 2022

Right, I think we should assume that both replica headers and info files are going to be working yaml since it is what lhapdf itself does.

@alecandido
Copy link
Copy Markdown
Contributor

Right, I think we should assume that both replica headers and info files are going to be working yaml since it is what lhapdf itself does.

Fine by me. I'd love not to depend on lhapdf for reading what's inside an info file.

@scarlehoff
Copy link
Copy Markdown
Member Author

Right, I think we should assume that both replica headers and info files are going to be working yaml since it is what lhapdf itself does.

Ok. Shall we do that in a separate PR? It will simplify #1537 if we also do it for postfit and anything else that might be writting info files but it might also break something (who knows) so better if we isolate it.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Apr 8, 2022

Right, I think we should assume that both replica headers and info files are going to be working yaml since it is what lhapdf itself does.

Ok. Shall we do that in a separate PR? It will simplify #1537 if we also do it for postfit and anything else that might be writting info files but it might also break something (who knows) so better if we isolate it.

Yes please.

@scarlehoff
Copy link
Copy Markdown
Member Author

Sorry, I've been an idiot.

I actually tested all pdfs in the LHAPDF server when I did lhapdf_management and I was able to read all info files with a standard yaml safe load

https://gitlab.com/scarlehoff/lhapdf/-/blob/master/python_management/test_itall.py#L60

I can rerun that, but I'm pretty sure I got no failing PDFs (on 09 Nov, 2021)

@alecandido
Copy link
Copy Markdown
Contributor

Maybe it was something specific @andreab1997 was doing. It might be that the problem was in the dump, not in loading them.

Can you reconstruct what you found @andreab1997?

@scarlehoff
Copy link
Copy Markdown
Member Author

I'll open an issue for this.

@scarlehoff scarlehoff mentioned this pull request Apr 8, 2022
2 tasks
@andreab1997
Copy link
Copy Markdown
Contributor

Maybe it was something specific @andreab1997 was doing. It might be that the problem was in the dump, not in loading them.

Can you reconstruct what you found @andreab1997?

Yes I think

@Zaharid Zaharid added the bug Something isn't working label Apr 8, 2022
@scarlehoff scarlehoff merged commit 1c86eb5 into master Apr 8, 2022
@scarlehoff scarlehoff deleted the hotfix_mc2hessian branch April 8, 2022 15:04
@alecandido
Copy link
Copy Markdown
Contributor

The fastest PR I've ever seen...

@scarlehoff
Copy link
Copy Markdown
Member Author

It was an obvious bug with an obvious solution. We've had faster :P https://github.com/NNPDF/nnpdf/pulls?q=head%3Ahotfix+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mc2hessian action doesn't write the correct error type

4 participants