-
Notifications
You must be signed in to change notification settings - Fork 13
Make the usage of feature scaling + large x a bit clearer #2401
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
Conversation
interpolation_points to feature_scaling_points ; now the n…|
Looking at the PR description yes, this is what we need. @kamillaurent can take a look and then try to run a fit without and with FS, everything else kept the same, to check that things work properly. I think it is useful that we can keep the two options for PDF parametrisation: for NNPDF4.1, probably we want to show in the paper the fit variants where everything is kept the same except for FS on or off |
47bd8d6 to
907dae5
Compare
|
@kamillaurent have you looked into this? Otherwise I'll rather close it and keep the current conventions, because the more this drags the most things are going to be broken and the more work it is for me to keep it. This should be something quick and trivial to review. |
…3fit checks are much more strict: no largex exponent is allowed when feature scaling is on, before they were silently dropped so no change of functionality is introduced by this PR, only change of checks ; vp-nextfitruncard automatically changes interpolation_points to feature_scaling_points and will drop largex if it is found together with feature_scaling_points
907dae5 to
0b8aec9
Compare
|
Hi @scarlehoff , I am looking at it now. Sorry for the late reply, I will get back to you as soon as I finish |
|
@scarlehoff I tried to run the runcard in the branch |
|
Hi @kamillaurent, by design I wanted to remove
and I would prefer if we do not mix both types of fits but I guess that ship has sailed anyway so the commit below puts the beta to NaN for the report if the fit has feature scaling on. |
|
Thanks for clarifying, the same error message appears also if I try to compare the fit done with feature scaling and the same fit iterated, I try now to do with the new commit:
Let me know if you have other checks to suggest |
That should not have happened instead!
The important one is actually that you iterated the fit (so But this PR was also about making the feature scaling part clearer so that's the important part. If you think it is indeed clearer, then good, otherwise (if the previous version seem clearer) better to roll it back. |
Yes, the fit (and comparisons) now work using runcards without
I think it is clearer, because you specify in the runcard |
kamillaurent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fit (and comparisons) now work using runcards without largex (both iterated fit and normal one), here are the reports:
- Test of a fit with this branch compared to baseline: https://vp.nnpdf.science/GiPUMdXHSwu8gjCeUFSjPQ==/
- Test of iterated fit: https://vp.nnpdf.science/ECbdZZvTSaKnnHF_D5LSWg==/
In the runcard, what we do is clearer since we specify feature_scaling_points
|
Thanks! Then on Monda/Tuesday I'll merge both this and @jekoorn's PR so that the Christmas hyperopt is run already with this change. Thank you very much for your work! |
Closes #2377
This PR deals with the clarity issues pointed out by #2377
interpolation_pointstofeature_scaling_pointsso that it is clear when it is set toFalseor not set at all.@kamillaurent @juanrojochacon let me know whether this is what you had in mind. This changes nothing on how the fit is run, right now the fit will just drop silently the
large-xif feature scaling is set, now instead if you have large-x set it will refuse to run until you remove it.