n3fit - Rationalized stopping-validation module #512
Conversation
scarlehoff
force-pushed
the
n3fit-refactor-stopping
branch
from
96d2c36
to
93ba56a
Compare
|
This is basically finished. Problems:
Issue:
Note: I'll rebase master as soon as n3fit-ProductionVersion is merged. |
scarlehoff
force-pushed
the
n3fit-refactor-stopping
branch
from
a04e12d
to
2aca8eb
Compare
…ith the full model
scarlehoff
force-pushed
the
n3fit-refactor-stopping
branch
from
2aca8eb
to
40bad0d
Compare
scarlehoff
changed the title
scarlehoff
changed the title
…ning and validation regardless of possible extra stuff that might be added
|
On Wed, Sep 18, 2019 at 4:49 PM Juacrumar ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In n3fit/src/n3fit/msr.py
<#512 (comment)>:
> @@ -120,7 +122,7 @@ def pdf_integrand(x):
)
-def compute_arclength(fitbasis_layer, verbose=False):
+def compute_arclength(fitbasis_layer):
Maybe we should factor out the part that actually does the computation and
share it.
yes, having common code would be good. Should be a different issue/PR
though. Feel free to tag n3fit in #156
<#156> as I agree both changes
should go together.
Ok will do.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#512?email_source=notifications&email_token=ABLJWUUDCZVWD5HXNVNB4E3QKJEZNA5CNFSM4IDZBT3KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFEWVSA#discussion_r325755932>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABLJWUVF3ZXPBK3XESDMUDTQKJEZNANCNFSM4IDZBT3A>
.
|
I apologize because it is so obvious this was the right way to do it I feel bad for having just hacked in the history object thing. Although for some reason I was very happy with the code the first time. Now, even if it is the right way, how to do it might be open to opinion. I have an idea of how to restructure the code in order to make both of us happy, but before I want to clarify several points in order not to make anyone re-review the code several times.
What do you mean exactly by this?
This will have to wait until there is an python object I can pass down that know whether it is a positivity set or not. As far as I can see I cannot do that in a clean manner (let me know if I am missing some
Saving snapshots will need to be outside of the scope of this PR because it requires quite a few changes at the
Only we cannot (and want not) save everything iteration by iteration. This is more a clarification than a comment. |
|
On Thu, Sep 19, 2019 at 12:19 PM Juacrumar ***@***.***> wrote:
As said elsewhere, I would design this differently [...]
Instead I would write all relevant information in the history object.
I apologize because it is so obvious this was the right way to do it I
feel bad for having just hacked in the history object thing. Although for
some reason I was very happy with the code the first time.
One thing that makes me think that holding all the mutable state in one
place (and reducing it to the minimum) is a good idea is the bad
experience with nnpdfcpp. I found it incredibly hard to implement a
slightly different strategy for fitting positivity because the required
bits and pieces lived in different classes that didn't talk to each other
while other parts needed to be separated out and follow different code
paths (I think I ended up putting a lot of code related to positivity in
Stopping because it was where the required information was). I ended up
rewriting a large part of the code (in a way that wasn't necessarily an
improvement) and we will never know if the failure of that project was due
to a bug I had introduced. I think this is bound to happen when you have
large classes that do a lot of stuff and you want to do something they
weren't explicitly intended for (e.g. see also how difficult it was to add
the theorycovmat to nnpdfcpp).
By contrast the overarching idea of validphys is that things should be put
together only at the last moment when they are needed. This then allows to
swap pieces of functionality around (see the explict_node thing) no matter
how deep they are in the code paths, and also e.g. add parameters to be
read from the runcard trivially (literally, just add a parameter to your
action). And it does it in a way that allows to have very high level
interface (the runcard or the API) at the same time.
In practice vp is far from ideal in that regard at the moment, because it
starts from the weird interdependencies of libnnpdf and also because it
doesn't always use "modern" reportengine features, which were implemented
or discovered after the fact. Also because most of these new features are
regretful hacks that I don't understand myself, but that's a different
story.
Now, even if it is the right way, how to do it might be open to opinion. I
have an idea of how to restructure the code in order to make both of us
happy, but before I want to clarify several points in order not to make
anyone re-review the code several times.
Similarly I'd model thing like Positivity as pure functions that operate
on the history object.
What do you mean exactly by this? check_positivity is basically a pure
function now.
Positivity as a class makes sense because we might want to have different
threshold (or even different conditions) and we might actually change the
conditions on the go. The flexibility is there and should not go. Maybe I
misunderstood the idea.
I'd say it is fine to have an object with no mutable state (which would be
the threshold only), but the positivity_ever_passed thing should be a
property of the state (or the history or whatever). So id do something like
#actually some name I can't think of right now to avoid confusion with vp
checks
def check_positivity(positivity_predictions: Iterable, threshold: float) ->
bool:
...
and
state.passes_possitivity = check_positivity(..., )
#For example, if needed
history.positivity_ever_passed |= state.passes_possitivity
or save the threshold in a class and define the method check_positivity if
you prefer.
There is nothing preventing us from having datasets starting with pos or
total. I think it would be better to have some kind of structure that
separates these things explicitly rather than interpreting names.
This will have to wait until there is an python object I can pass down
that know whether it is a positivity set or not. As far as I can see I
cannot do that in a clean manner (let me know if I am missing some
is_positivity funtion) so I don't know how to solve this problem cleanly
without adding things way outside of the scope of this PR.
Not sure I understand. Positivity is a different thing in vp already:
https://github.com/NNPDF/nnpdf/blob/f9f753b1a7e1ad579b5ef686ed6f3bb229cde4e6/validphys2/src/validphys/core.py#L442
… Then saving a snaphot is spelled pickle.dump(fitstate, "file.pkl") and
this method would not mutate the history but yield frames instead.
Saving snapshots will need to be outside of the scope of this PR because
it requires quite a few changes at the backend level. This is something I
have on mind because currently the save/load mechanism is crap.
There is FitState which contains all the things that change iteration by
iteration
Only we cannot (and want not) to save everything iteration by iteration.
This is more a clarification than a comment.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#512?email_source=notifications&email_token=ABLJWUWRCB7ER4P4WD2KY2DQKNN4ZA5CNFSM4IDZBT3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7DD5HQ#issuecomment-533085854>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABLJWUWQOUESYORYT5CBBQ3QKNN4ZANCNFSM4IDZBT3A>
.
|
|
I agree with everything
The example of Instead, the cleanest way to do it (in my mind) is to have all specs to have a *you might say "this is not worse than what you currently have" but I don't see the point of changing it to something that is obviously flawed as well. Even if the flaw is somewhat smaller. |
|
On Fri, Sep 20, 2019 at 1:16 AM Juacrumar ***@***.***> wrote:
I agree with everything
Not sure I understand. Positivity is a different thing in vp already
The example of is_positivity was a very bad one. I really don't want isinstance(thing,
PositivitySpec) because I don't feel it is an improvement.
In the following sense:
we want only the loss of the experiments to go in the chi2 as per #543
<#543> then the logical thing is to
select those object which are of the type ExperimentSpec, but then the
problem is that if we suddenly start fitting other objects (like datasets
or other type of experiments) the code has potential for failing.
Similarly, if we check for which types we don't want (i.e.,
PositivitySpecs) if at some point we add something with a loss
RegularizationSpec the code will fail for sure.*
The one example we have for this right now is rather unambiguous:
positivity is clearly different from dataset in that it doesn't have data
measurements associated. There is very little point in pretending they are
similar thing.
Instead, the cleanest way to do it (in my mind) is to have all specs to
have a count_me_in_the_total_chi2 which can default to False and only be
true for experiments and datasets (for instance).
*you might say "this is not worse than what you currently have" but I
don't see the point of changing it to something that is obviously flawed as
well. Even if the flaw is somewhat smaller.
I think that having an object that explicitly encodes the relevant
information is obviously better than one where you have to parse the string
names (in a way that is rather dangerous in that it relies on unspecified
conventions). Feels like reading an external format when in fact it is
something we control on both ends.
Regarding various possible generalizations, I think we should deal with
them once we need them, but it seems to me that what you fit how is a
property of the fit rather than of the corresponding container. So *if*
there was the need, there would be some option in the runcard affecting how
the input in the fit is constructed.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#512?email_source=notifications&email_token=ABLJWURSQKP36OKD765PCK3QKQI5VA5CNFSM4IDZBT3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7FFR7A#issuecomment-533354748>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABLJWUTN6DRKW6IQYDJLZN3QKQI5VANCNFSM4IDZBT3A>
.
|
|
To me it looks like the "correct" way is too easy for me to leave a hard-coded flawed approach in just because the one example we have now is unambiguos. If you think it is not a good idea doing so right now at the level of vp, I'll do it at the level of |
|
But what are the other (imaginated) possibilities? It seems to me
that the fit has to have some concept of dataset and some concept of
positivity that you put it manually. Different things (whatever they
might be), you will have to put in manually in a different way. I
guess all I am saying is that right now, I think we should be trading
something more structured than a dictionary what we have to do various
kinds on string processing upon. We have more adequate ways to
represent the input than prefixes in dictionary keys.
…On Fri, Sep 20, 2019 at 12:36 PM Juacrumar ***@***.***> wrote:
To me it looks like the "correct" way is too easy for me to leave a hard-coded flawed approach in just because the one example we have now is unambiguos.
This will unevitably lead to less flexibility, which I'd rather avoid (when I am lucky enough to catch it, of course).
If you think it is not a good idea doing so right now at the level of vp, I'll do it at the level of reader.py which for my purposes are the same thing.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
We agree on that. I'm just saying I don't want to do a type check (or ducktyping) four levels down when they are different things already at the top-level (be it vp or reader.py) and so what the Of course it has to be written manually, I was just wondering whether this information should already come from vp. If not I'll put it manually in the reader.
For instance, if I want to fit a positivity set. Being real or not is beside the point, the design principle is full flexibility whenever possible so from two options* we chose the one that offers more fkexibility, even if the use case is not there. *comparable options in the required amount of work |
So I guess I am not sure what we are discussing amount anymore :D.
Indeed vp should provide you with a set of data and another set of positivity constraints, but then again it already does that, no? They are not mixed up anywhere AFAICT. |
I'm just trying (and failing) to get my point across. I'll first make this change (the way I have in my head) and then I'll do the fitstate. The way I have in my head is not incompatible with yours and I think it is an improvement. If once it is implemented you like it it can be propagated up to vp in whatever way is convenient. |
|
I have two different keys in the dictionaries: I use At the moment effectively Furthermore, adding an extra boolean flag to a dictionary is the least harmful of things. I hope with the code what I failed to explain in several previous comments is much clearer (have a look just a |
|
From a quick look, seems a lot better now! Will try to go over it ASAP. |
| return tr_ndata_dict, vl_ndata_dict, pos_set | ||
|
|
||
|
|
||
| class FitState: |
There was a problem hiding this comment.
Could these go in a different file? I'd not search for them in something called stopping.
There was a problem hiding this comment.
But they are only used in this file so if you are looking for them you already have stopping.py opened.
There was a problem hiding this comment.
They are only used inside this class.
Don't really care much, but I think it is cleaner in the same file under the rationale "if you are looking for these classes, you have already opened this file"
There was a problem hiding this comment.
I guess if we had started from this, the stopping functionality would be inside or around it, i.e. it would belong to the fit rather than stopping. But fine.
n3fit/src/n3fit/stopping.py
Outdated
| # a terrible run, mark it as such | ||
| self.terrible = True | ||
|
|
||
| def __iter__(self): |
There was a problem hiding this comment.
I think this should work differently. Most importantly I know of no python object, either in the stdlib or in some famous library where next(iter(object)) mutates object, and I'd find that confusing if I encountered it. There is little point in yielding i because you can always call enumerate on the thing. Finally, is this used anywhere at all (in the proper case e.g. as a for loop)?
I'd either remove this or have it simply yield the states. A method called something like rewind(self, frame_index) -> new history might be useful though.
There was a problem hiding this comment.
But you do nothing with the states, that's why I am just returning the step index.
Maybe it makes more sense to have a two steps thing where you would just do
for i in range(history_size):
history.rewind(step = i)
do whatever with your new reloaded thing
?
There was a problem hiding this comment.
It is a bit tricky because there is really no need to return anything, so the options are:
for i in history:
# do thing
where the loop mutates history and you just receive a counter or,
for step in history.length:
history.rewind(step)
# do thing
so that the mutation of history is done explicitly. I'm changing to the second version.
There was a problem hiding this comment.
Damn it, I though the previous comment was never sent. I blame unimi's eduroam. I've commited this change now. It is uglier than before (to me) but leaves little to the imagination.
n3fit/src/n3fit/stopping.py
Outdated
| # Arguments: | ||
| - `validation_model`: a reference to a Validaton object | ||
| this is necessary at the moment in order to save the weights | ||
| - `save_each`: if given it will save a snapshot of the fit every `save_each` epochs |
There was a problem hiding this comment.
Also save_each is confusing: save_weights_each or something like that.
Furthermore, is there a case for even having two lists? Couldn't we save the chi2s less frequently but at the same time as the weights?
There was a problem hiding this comment.
The weights are heavy.
You can save the chi2 less frequently but you still need to save the chi2 every X epochs if you want to write logs down and the logs are not necessarily in sync with the best-weights.
You can set it so that they are only saved when debug:true but you would still be forced to have the two-lists implementation in...
…it with a rewind(step) method
|
Hello, this is @Zaharid's automated QA script. Please note it is highly experimental. I ran pylint on your changes and found some new issues. On
On
On
On
On
On
On
|
| return tr_ndata_dict, vl_ndata_dict, pos_set | ||
|
|
||
|
|
||
| class FitState: |
There was a problem hiding this comment.
I guess if we had started from this, the stopping functionality would be inside or around it, i.e. it would belong to the fit rather than stopping. But fine.
|
Thanks! I went over this and I am happy with it. Do we have tests that show that it works fine, especially in things that are tricky such as rewinding the history? |
|
Nothing systematic yet. |
As promised, this PR basically breaks down the huge mess that was Stat_info into several classes for stopping, validation and positivity, making things a bit (don't want to oversell it) clearer.
There are a few things to do yet, when I finish them I'll ask for reviews.
Change the way the validation is checked at the end of every epochThe point of this PR:
The main point of this changes was making the previously-named Statistics.py readable
(now called stopping.py) so anything that still looks convoluted/unreadable after the commits should be changed.
Example code for animation.py: animation.tar.gz