Skip to content

Use python CommonData#1650

Merged
scarlehoff merged 7 commits into
masterfrom
use_only_python_commondata
Jan 20, 2023
Merged

Use python CommonData#1650
scarlehoff merged 7 commits into
masterfrom
use_only_python_commondata

Conversation

@scarlehoff
Copy link
Copy Markdown
Member

@scarlehoff scarlehoff commented Dec 15, 2022 •
edited
Loading

There has been a half-finished, half-used python version of the CommonData object for a while

nnpdf/validphys2/src/validphys/coredata.py

Line 202 in 9d57bb0

class CommonData:

This PR completes so that it can be used everywhere.

This still doesn't allow to remove libNNPDF because the generation of closure test data is still done from libNNPDF. Is there someone in Cambridge dealing with that at the moment @Zaharid ?

If not I could also have a go at that after this is merged.

Note that I have not attempted to fix any possible design problems there could be with the python version of the CommonData or with how the libNNPDF was used. This is just a conversion so that when we move to the new commondata I only have to port one type of CommonData instead of two.

TODO

  • Check all validphys runcards in the example folder to check that I have not missed some corner action not covered by tests.

@scarlehoff scarlehoff requested a review from Zaharid December 15, 2022 14:51
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Dec 15, 2022

We are going to have a pr for the closure test data soon.

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Dec 15, 2022

Good. I guess is this branch? https://github.com/NNPDF/nnpdf/compare/python_closure_sampling

If so there seem to be no overlap with the changes of this PR so when both are merged it should be possible to remove libNNPDF at last.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Dec 15, 2022

Cc @comane

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Dec 15, 2022

Would be good if the export functions were more layered, providing ways to dump the individual files to any streams, mirroring what we have in commondataparser. Among other things, this help us to test easily that writing and reading roundtrip. These lower level things might go in commondata parser.

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Dec 15, 2022

I didn't check whether they were the same but are they not included in the #1651? Or those are yet something different which is closure test only?

@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Dec 15, 2022
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Dec 15, 2022

I guess that part does overlap.

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Dec 15, 2022

Yes, but it is not destructive overlap. When that one is merged the export can be changed to use the two functions and done!

@scarlehoff scarlehoff added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Dec 16, 2022
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Dec 16, 2022

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Dec 16, 2022
@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Dec 16, 2022

This is now ready for review. As far as I can test there's nothing broken.

Added @andreab1997 as a reviewer in case this touches the covmat in some way. I think it shouldn't because the thcovmat was freed from libNNPDF, but just in case.

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Jan 4, 2023

@Zaharid did you have a look at this?

@andreab1997 please, have a look as well

@scarlehoff scarlehoff mentioned this pull request Jan 12, 2023
Merged
@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Jan 12, 2023 •
edited
Loading

With #1660 it should be possible to merge this to master without creating conflicts? Hopefully?

(although @Zaharid, as you see in #1660 and I hope you believe me now, the merge is trivial... so feel free to close #1660 and keep working with @comane in his branch?)

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Jan 17, 2023

I'd like to merge this by the end of the week if there are no blocking issues with it.

I'd create a tag before the merge as the last version that still uses libNNPDF in the python code since I expect we will be able to remove all libNNPDF imports from validphys after this one and #1651 (or #1660) are merged.

alecandido
alecandido reviewed Jan 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

It is fine as it is, apart from LegacyCommonData still being used.

All the other comments are mainly to improve style and consistency, but do not consider them blocking.

Comment thread validphys2/src/validphys/commondataparser.py Outdated
Comment thread validphys2/src/validphys/commondataparser.py Outdated
Comment thread validphys2/src/validphys/commondataparser.py Outdated
Comment thread validphys2/src/validphys/commondataparser.py Outdated
Comment thread validphys2/src/validphys/commondataparser.py
Comment thread validphys2/src/validphys/coredata.py
Comment thread validphys2/src/validphys/coredata.py
Comment thread validphys2/src/validphys/coredata.py
Comment thread validphys2/src/validphys/filters.py
Comment thread validphys2/src/validphys/filters.py Outdated
scarlehoff and others added 2 commits January 17, 2023 12:48
@scarlehoff @alecandido
Apply suggestions from code review
609d6c8 
Co-authored-by: Alessandro Candido <candido.ale@gmail.com>
@scarlehoff
apply review comments
c8e9cf6
@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Jan 17, 2023

RE LegacyCommonData, while the fit is not using it, there are too many load scattered around the code. Once the pseudodata one is merged and I'm sure that no libNNPDF should be use I'll remove all of them.

If then there are functions that are just there and not being used (it wouldn't be the first time) I'll just remove them.

Zaharid
Zaharid reviewed Jan 17, 2023
View reviewed changes
Comment thread validphys2/src/validphys/commondataparser.py Outdated
@Zaharid Zaharid added the run-fit-bot Starts fit bot from a PR. label Jan 17, 2023
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Jan 17, 2023

I am inclined to merge this as is, provided various tests pass.

@RoyStegeman
Copy link
Copy Markdown
Member

RoyStegeman commented Jan 17, 2023

Please give me this afternoon to have a look at it. Mainly just so I know exactly what this PR does, but might as well take the opportunity to provide some comments if I have any.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 17, 2023

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

RoyStegeman
RoyStegeman reviewed Jan 17, 2023
View reviewed changes
Comment thread validphys2/src/validphys/commondataparser.py Outdated
Comment thread validphys2/src/validphys/coredata.py Outdated
Comment thread validphys2/src/validphys/coredata.py
Comment thread validphys2/src/validphys/dataplots.py Outdated
scarlehoff and others added 2 commits January 17, 2023 23:21
@scarlehoff @RoyStegeman
Apply suggestions from code review
a159219 
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
@scarlehoff
kinnames -> kin_names
df13d18
@scarlehoff scarlehoff mentioned this pull request Jan 18, 2023
Merged
@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Jan 20, 2023
@scarlehoff scarlehoff merged commit f9f0780 into master Jan 20, 2023
@scarlehoff scarlehoff deleted the use_only_python_commondata branch January 20, 2023 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Zaharid Zaharid Zaharid left review comments

@RoyStegeman RoyStegeman RoyStegeman approved these changes

@andreab1997 andreab1997 Awaiting requested review from andreab1997

+1 more reviewer

@alecandido alecandido alecandido approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

destroyingc++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@scarlehoff @Zaharid @RoyStegeman @alecandido