Substitution of apfel with eko in evolven3fit #1537
Conversation
|
We have a little issue that maybe we want to discuss. Since the function which is actually doing the evolution knows nothing about the number of final replicas (since it is doing the evolution exportgrid wise) we do not have this information to put on the info file (numreplica). What should we do then? Should we give it another argument (num_replicas)? However in that case this means that the CLI and/or the initial runcard must have the information which maybe is not what we want @scarlehoff |
The Edit: currently |
|
You can add now Also don't modify the current Edit: actually, make sure that (since you are also developing |
scarlehoff
force-pushed
the
evolfit_w_eko
branch
from
9ce0ab4 to
c0ddad3
Compare
scarlehoff
added
run-fit-bot
|
Greetings from your nice fit 🤖 !
Check the report carefully, and please buy me a ☕ , or better, a GPU 😉! |
|
Now that the test are passing, I guess this can be merged to master? Is there any extremely important points that need to be fixed? I would like to have this merged by next week because I totally need this branch for the CMS tutorial in two weeks (if a fit is to be done, I cannot make people wait for one hour to evolve the result... so I have strong reasons to merge it even if it is still not perfect). |
|
@scarlehoff For me we can merge this if you need it this week. |
|
Yes, for me it is ok. Let's give people until tomorrow during the CM in case anybody wants some (very) specific changes. |
RoyStegeman
left a comment
RoyStegeman
left a comment
There was a problem hiding this comment.
Didn't really review, just two things I noticed as I scrolled through the changes. Given the huge number of commits relative to the changes in this PR, I just wanted to ask if git blame of other files has not been changed somewhere along the way?
| #conda-build | ||
| conda_bld/ | ||
|
|
There was a problem hiding this comment.
Do we want this here? I don't even call my build folders this
There was a problem hiding this comment.
I actually would also leave the first one! It's .gitignore, I'm fine ignoring anything people find useful.
| #without this | ||
| numpy: | ||
| - 1.19 | ||
| - 1.21 |
There was a problem hiding this comment.
I don't know whether it is necessary but I'm not touching conda with a stick in the next three months.
|
It really doesn't get more specific than https://github.com/NNPDF/nnpdf/pull/1537/files#r1036915773 |
|
@Zaharid are you ok to merge this now? |
Zaharid
left a comment
Zaharid
left a comment
There was a problem hiding this comment.
I am not going to keep blocking this but I stand by the claim that the way the LHAPDF grid management works here is deeply flawed and should have been fixed if this project is so urgent. Please open an issue against the library you are using to introduce the interfaces that are clearly needed here, and, upon merging this, another issue in this repo to use the internal apis.
That said, there were several people with other substantive points that it is not clear to me that have been addressed (there are several people with "requested changes" and "comment" status). Indeed it would be good if someone could conduct a proper review and positively approve this.
|
Before keep going with reviews and so on, if we are any close to merge, please rebase on master (GitHub is crying for potential conflicts) |
scarlehoff
left a comment
scarlehoff
left a comment
There was a problem hiding this comment.
Let's merge this. If nobody stops me explicitly I'll merge it tonight.
If everything explodes we'll live with it.
8 participants
The idea is to have eko perform the evolution (so also avoiding to recompute the operator for each replica). Moreover in this way we will be able to run it automatically at the end of each replica production by n3fit (leaving however the possibility to run the CLI after n3fit).