Skip to content

In process_function take nnpdf31_process from PLOTTING_*.yaml#1472

Merged
Zaharid merged 4 commits into
masterfrom
fix_proc_lookup
Dec 1, 2021
Merged

In process_function take nnpdf31_process from PLOTTING_*.yaml#1472
Zaharid merged 4 commits into
masterfrom
fix_proc_lookup

Conversation

@RoyStegeman
Copy link
Copy Markdown
Member

addresses #1288

@RoyStegeman RoyStegeman requested a review from Zaharid November 19, 2021 09:45
@RoyStegeman RoyStegeman added the bug Something isn't working label Nov 19, 2021
@RoyStegeman RoyStegeman changed the title Fix proc lookup In process_function take nnpdf31_process from PLOTTING_*.yaml Nov 19, 2021
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Nov 19, 2021

A couple of things need to be done here:

  • We have higher level functions for recovering the plotting file information, specifically validphys.plotoptios.get_info. See e.g.

    metadata = get_info(cd)
    Also ideally this would not touch a Loader and instead recover a commondata object, but looking at the uses of this function that might be involved.

  • In the code triggering the warnings in comparefits, specifically

    (process_lookup(dsin.name), dsin.name): dsin for dsin in data_input
    we should definitively use the get_info lookup (also I was not too happy about adding anything related to processes there but not looking at what that breaks).

@RoyStegeman
Copy link
Copy Markdown
Member Author

we should definitively use the get_info lookup

Sure, in the config Class everything is already there to do that instead of the process_lookup, but that's not the case for the other instances of process_lookup.

Also ideally this would not touch a Loader and instead recover a commondata object

Not really sure what you mean here. A commondata object is alsways obtained through a Loader at some stage, right? Both the lines you point at are inside CoreConfig, while process_lookup is called outside that class in most cases so the commondata object is not as readily available there.

@RoyStegeman
Copy link
Copy Markdown
Member Author

@Zaharid could you clarify your points above?

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Nov 24, 2021

we should definitively use the get_info lookup

Sure, in the config Class everything is already there to do that instead of the process_lookup, but that's not the case for the other instances of process_lookup.

Also ideally this would not touch a Loader and instead recover a commondata object

I meant that line specifically, which is used in a lot of code paths.

Not really sure what you mean here. A commondata object is alsways obtained through a Loader at some stage, right? Both the lines you point at are inside CoreConfig, while process_lookup is called outside that class in most cases so the commondata object is not as readily available there.

I like the idea of having some separation of concerns between what happens before and after the graph runs. Stuff like funding appropriate paths to commonda happens before as of now. On top of the (arguable) theoretical purity, it does enable certain features, like for example allowing to override the commondata lookup for the purposes of closure tests. That works because there is a single point that knows how to get the commondata(spec) and knows how to handle that keyword. I suppose it is mostly inconsequential for the remaining uses, but would be good if it wasn't needed.

@Zaharid Zaharid merged commit a158756 into master Dec 1, 2021
@Zaharid Zaharid deleted the fix_proc_lookup branch December 1, 2021 15:23
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.

2 participants