Skip to content

fixed bug following from introduction of parse_fakepdf in config.py#1654

Merged
Zaharid merged 7 commits into
masterfrom
fakepdf_bug_vp_comparefits
Jan 3, 2023
Merged

fixed bug following from introduction of parse_fakepdf in config.py#1654
Zaharid merged 7 commits into
masterfrom
fakepdf_bug_vp_comparefits

Conversation

@comane
Copy link
Copy Markdown
Member

@comane comane commented Dec 23, 2022

Because of the introduction of the parse_fakepdf method in config.py, the val corresponding to the "fakepdf" key of the datacuts dict in produce_fitunderlyinglaw function is not type str anymore but core.PDF. This leads to an error when running vp-comparefits:

[ERROR]: Bad configuration encountered:
Bad input type for parameter 'pdf': Value 'NNPDF40_nnlo_as_01180' is not of type str, but of type 'PDF'.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Dec 24, 2022

Do we need to parse the pdf again or can we set it to the value we already get?

@Zaharid Zaharid added the bug Something isn't working label Dec 24, 2022
Copy link
Copy Markdown
Member

@RoyStegeman RoyStegeman left a comment

Choose a reason for hiding this comment

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

Thanks for pointing this out. I agree with Zahari, I don't see why we need to parse it seperately in each function, in this case we should just use the pdf directly.

@comane comane requested a review from RoyStegeman December 27, 2022 09:51
@comane
Copy link
Copy Markdown
Member Author

comane commented Dec 27, 2022

Thank you for the comments. The new commit should do so. The fakepdf stored in datacuts is taken instead of parsing it again.
There might be other issues with the config.produce_multiclosure_underlyinglaw() function but I don't know in which context this function is to be used.

Comment thread validphys2/src/validphys/config.py Outdated
@RoyStegeman
Copy link
Copy Markdown
Member

There might be other issues with the config.produce_multiclosure_underlyinglaw() function but I don't know in which context this function is to be used.

What problems are you anticipating? I guess that function would be used to calculate closure test estimators such as these: https://vp.nnpdf.science/5m-aHiGmRVGsOjeZ58AtyA==/ which are based on multiple fits (25 in this case) and to correctly evaluate these estimators the underlying law for each of the fits needs to have been the same.

Zaharid and others added 3 commits December 28, 2022 16:13
…nto fakepdf_bug_vp_comparefits

minor modification in validphys.config.produce_fitunderlyinglaw
@comane
Copy link
Copy Markdown
Member Author

comane commented Dec 28, 2022

What problems are you anticipating? I guess that function would be used to calculate closure test estimators such as these: https://vp.nnpdf.science/5m-aHiGmRVGsOjeZ58AtyA==/ which are based on multiple fits (25 in this case) and to correctly evaluate these estimators the underlying law for each of the fits needs to have been the same.

I guess the problem could be that
laws.add(closuretest_spec["fakepdf"])
is adding a PDF object to the laws set instead of a string, therefore
return self.parse_pdf(laws.pop())
would give the same problem as above.

I think one could just do the following change:
return laws.pop()

@RoyStegeman
Copy link
Copy Markdown
Member

Did you try? I don't think it's a problem here because the fakepdf is obtained by reading the filter.yml file directly (i.e. it's not parsed by reportengine and thus not placed in a namespace).

@comane
Copy link
Copy Markdown
Member Author

comane commented Jan 3, 2023

Yes, I agree with you. Thanks for the comment. I think with the last commit the branch can be merged

Copy link
Copy Markdown
Member

@RoyStegeman RoyStegeman left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for spotting this!

@Zaharid Zaharid merged commit cee3479 into master Jan 3, 2023
@Zaharid Zaharid deleted the fakepdf_bug_vp_comparefits branch January 3, 2023 12:19
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Jan 3, 2023

Thanks both!

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.

3 participants