search

NNPDF / nnusf

An open source machine learning framework that provides predictions for all-energy neutrino structure functions.
https://nnpdf.github.io/nnusf/
GNU General Public License v3.0
0 stars 0 forks source link

Work on sffit #12

Closed RoyStegeman closed 2 years ago

RoyStegeman commented 2 years ago

This builds on #10. It adds early stopping, saving of the model and merges updates to main.

RoyStegeman commented 2 years ago

I'm wondering if we should first invert the covmat before splitting it (pretty sure that's what's done in n3fit)

RoyStegeman commented 2 years ago

Also, the chi2 is calculated per experiment during the fit. This is the thing I pointed out in the other PR, but then thought that I was wrong because it looked like you were dividing by the total number of point in all datasets (you're not though)

RoyStegeman commented 2 years ago

We should also decide whether we want to use callbacks or your for loop. I'm happy with either, but if we go with the latter callbacks.py can be removed.

Radonirinaunimi commented 2 years ago

We should also decide whether we want to use callbacks or your for loop. I'm happy with either, but if we go with the latter callbacks.py can be removed.

I am also happy with both. But maybe for the flexibility in presenting the output logs we can lean towards the for loo approach? We can just write a nice function that controls the stopping.

RoyStegeman commented 2 years ago

I am also happy with both. But maybe for the flexibility in presenting the output logs we can lean towards the for loo approach? We can just write a nice function that controls the stopping.

Yes I agree. As long as there's as little functionality added as we have now I think the for loop is more readable. Feel free to remove the callbacks.

RoyStegeman commented 2 years ago

@Radonirinaunimi I think everything is here now, right? Shall I review and then we can merge? Other smaller updates we can do in separate PRs.

Radonirinaunimi commented 2 years ago

@Radonirinaunimi I think everything is here now, right? Shall I review and then we can merge? Other smaller updates we can do in separate PRs.

Yes! We definitely should, I was exactly thinking about asking you this. Sot, let's merge this PR.

RoyStegeman commented 2 years ago

Okay, I followed your commits one-by-one, but allow me a little time to go over it in detail again.

RoyStegeman commented 2 years ago

@Radonirinaunimi can I run black and merge?

Radonirinaunimi commented 2 years ago

@Radonirinaunimi can I run black and merge?

Yes, please do so! (I already ran black before but there are indeed surely parts that I missed)