Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fit runcards should not have innecesary information #536

Closed
scarlehoff opened this issue Aug 29, 2019 · 5 comments
Closed

Fit runcards should not have innecesary information #536

scarlehoff opened this issue Aug 29, 2019 · 5 comments
Labels
enhancement New feature or request n3fit Issues and PRs related to n3fit

Comments

@scarlehoff
Copy link
Member

There are various places where the runcards are checked to ensure they contain all necessary information, for instance fitting['nnodes'] or fitting['seed']. These two, for instance, don't make sense for n3fit.

Furthermore, as was proposed for the exp data, we might want to set some defaults which get written to the runcard within the fit directory but are not necessary in the fit runcard.

A practical example: a runcard without seeds.
This runcard should run ok but the filter.yml runcard inside the directory should have all seeds set from some defaults such that the run can be reproduced.

Pros:

Writing a fit runcard will become easier and easier.

Cons:

Maybe it is preferable for fit runcards to be always as comprehensive as possible and then this proposal is a really bad idea.

Note: I think this is not a new idea as I can see some issues where some forms of this were discussed (#402 or #496) but it was always in the context of validphys actions and I am not sure the fit runcard abides by the same rules. I preferred to open a new issue which addresses this specifically.

@scarlehoff scarlehoff added enhancement New feature or request n3fit Issues and PRs related to n3fit labels Aug 29, 2019
@Zaharid
Copy link
Contributor

Zaharid commented Aug 29, 2019

I think the discussion in #496 broadly applies to this indeed. I'd say we want to make writing and reading and checking* runcards for the default happy case as simple as possible. I have to say having to write the seeds manually has always bothered me. The lock files could generate be used to stored them, when needed.

@scarlehoff
Copy link
Member Author

I want to tackle this as I start removing things from n3fit (such as the exporting of the arclengths or the pdf grids), so I have some questions @Zaharid @wilsonmr @scarrazza
My goal would be to have a minimal functioning runcard.

  1. Why is the closure test dictionary necessary to run a fit even when we are not doing a closure test. I think this is not used, as I just created a runcard with a fake closuretest entry with random input and went through...

  2. Related to point 1, when running vp-setupfit I do need the closuretest dictionary in order to ask whether fakedata is true or not. Then even when it is not true, it keeps looking for some of the other entries in the closuretest dictionary. Is this a good idea? (meaning, I have to define some closure test even if it is just with random value to run a fit)

  3. Talking about vp-setupfit, what is it actually used for? Maybe it should be part of postfit in the case of n3fit since it is not actually necessary to run a fit... but even so, the requirements for the runcard in both cases should be the same.

  4. I am not using at all the lhagrid dictionary (the exportgrid part is hardcoded) so for now I'll remove it. Once I change the exportgrid part to the corresponding vp actions I'll guess I'll find out which elements need to go back in to the runcard.

That's it for now.

@wilsonmr
Copy link
Contributor

wilsonmr commented Apr 7, 2020

  1. It's used as a namespace at the moment in vp-setupfit:

'datacuts::closuretest::theory::fitting filter',

which means it must exist and look like a namespace (so I guess be a dictionary)

  1. probably not - I think that the filter action could properly be slightly improved

  2. at the moment closure tests need it to be ran in advance. Once we had all the data loading done in pure python I was going to make closure tests be done in a smarter way in n3fit - at the moment it's a bit of a hack so that we could get something stable with minimal changes

@Zaharid
Copy link
Contributor

Zaharid commented Apr 7, 2020

We could certainly fix closuretest in setupfit.
I am not sure if exportgrid is very useful indeed.

@voisey
Copy link
Contributor

voisey commented Dec 16, 2020

Closed via #973

@voisey voisey closed this as completed Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request n3fit Issues and PRs related to n3fit
Projects
None yet
Development

No branches or pull requests

4 participants