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

zfit loss and minimizer #41

Open
jonas-eschle opened this issue Oct 24, 2019 · 9 comments
Open

zfit loss and minimizer #41

jonas-eschle opened this issue Oct 24, 2019 · 9 comments

Comments

@jonas-eschle
Copy link

jonas-eschle commented Oct 24, 2019

As already discussed with @pelssers, I would propose you for the long term to make use of zfit, which contains API and workflow definitions for model fitting including implementations. This means that the loss can be simply wrapped by a zfit loss and then the zfit minimizers (they include Minuit) can be used. Furthermore, statistical treatments like confidence interval etc can be performed simply since statistics libraries build on top of the zfit interface.

The idea of this unification is to have a common effort in implementing, wrapping and improving minimizers and not everyone needs to become an expert. Furthermore, it may helps to structure the code more cleanly when separating the loss building and minimization process. In fact, the only task left to flamedisx is to build the loss.

However... currently, zfit is not fully TF2 eager compatible yet (since all other tf packages for fitting built on the graph mode, congrats to start out with the TF2 ;) the compatibility will come for sure). All that is basically needed though is:

  • a function that evaluates the nll (either explicitly depending on an input of parameters on implicitly on zfit parameters; the latter can be used just as tensors but are objects that can change the value, basically tf.Variable)

This works! We've already tested it at PyHEP, though a few hacks on zfit side (given it was not fully TF2 compatible) were necessary.

What are your thought on this? Do you have any requirements for parameters, minimization or similar that are may not fulfilled or critical?

@pelssers
Copy link
Member

I'd definitely be interested in seeing how zfit performs compared the our current implementation using a 'regular' optimizer (we now use either L-BFGS, BFGS or Minuit). I also see the benefit of using a more widely used fitting tool.
One thing I mentioned during PyHEP is that for statistical inference we also need non-asymptotic interval estimation since we are dealing with low number statistics and therefore hesse or even minos errors might not be correct and we might need a complete Neyman construction. We'd need to think about how to implement that using zfit.
It's great that we were able to get the zfit and flamedisx code to talk to each other at PyHEP, I'm looking forward to TF2 eager compatible zfit.

@jonas-eschle
Copy link
Author

jonas-eschle commented Oct 25, 2019

Good to know, I'll keep you up to date.

I'd definitely be interested in seeing how zfit performs compared the our current implementation using a 'regular' optimizer (we now use either L-BFGS, BFGS or Minuit).

It may performs pretty similar, the advantage is mostly that for improvements, the community works on the same minimizers. And it's more stable, probably better tested etcetc and maintained by zfit (one sorrow less ;) ). Plus the below:

One thing I mentioned during PyHEP is that for statistical inference we also need non-asymptotic interval estimation since we are dealing with low number statistics and therefore hesse or even minos errors might not be correct and we might need a complete Neyman construction. We'd need to think about how to implement that using zfit.

Yes, the good news is you don't need to implement that yourself (or if, then you implement it once for the whole community). The package lauztat does this already and the (emerged from that) upcomming scikit-stats (more stable, better designed) will do that (or be the place to contribute anything like that). It builds on top of the zfit interface, so using zfit for the loss and minimization will allow a seamless transition.

@pelssers
Copy link
Member

Hi @mayou36, we've just merged a large refactor of our inference code. It should now be easy to add additional backends such as zfit to flamedisx.
If you're interested you can have a look at:
https://github.com/FlamTeam/flamedisx/blob/master/flamedisx/inference.py#L109
which shows how SciPy.optimize.minimize is added as a backend by defining a subclass of Objective and writing a minimize function.

@jonas-eschle
Copy link
Author

(sorry for my late reply, it got lost! Don't hesitate to ping me)
That looks really good! There is though one thing, zfit is not really a backend but rather an API and workflow definition. Because the problem is that there are 10 different libraries that put up something quite similar to what you did. This means having a lot of duplications, different minimizer wrappers in different libraries etc.

To end this, either all 10 libraries converge to 1, which usually does not work since they were all built with the specific use-case in mind and "a specific group of people has technical control over it" (so why trust that they don't just tweak it for their case in the future?), or we build a new library and API definition, that is, by design, enough flexible to cover all the use-cases and belongs to the community, anyone can join. And this is zfit.

So while what you have looks really nice and is basically the same spirit as what we have in zfit for loss and minimizers, it is more general in zfit, having other use-cases in mind. The best case would be if you could adapt to the loss interface from zfit, then you can move all the minimizers out and just use what is in zfit.

Why: this greatly reduces code duplication and maintenance for everyone. Furthermore, it means that we can all work on the same wrapper of e.g. scipy minimizer.
What is clear: The things you're doing here have to work with the zfit minimizers! If not, it is a bug in zfit, since zfits whole reason to exist is to make this things work. Moving that will also reveal possible weaknesses in the current design of zfit (maybe), which we would of course address. Meaning, zfit, the common framework gets better as well.

(and, since anyone is welcome to join, as soon as something is contributed etc. credits for authorships or papers will of course be given; it's a community project)

Technical: I am still working on a full TF2 conversion. If you agree to wrap it to the zfit loss, I will of course help with the conversion, also making a first scetch if you like, and try to make it work, giving you the branch etc. that it should work with etc.

What do you think about this?

P.S: I know it's a "large request". In my view, it is though the only option to build a maintainable and stable Python ecosystem, guaranteeing that libraries like flamedisx will survive for as long as possible by minimizing any maintenance)

@jonas-eschle
Copy link
Author

jonas-eschle commented Feb 7, 2020

Hi all, I was wondering what's the status of this, do you have an opinion?

@JelleAalbers
Copy link
Member

Hi @mayou36, thanks for your interest! If I understand correctly, your suggestion is to wrap the flamedisx likelihood in a zfit loss, then build any statistics code on top of zfit, or contribute our inference code to existing packages that do so.

I think this is a great idea: there's no need for every experiment to develop, debug and maintain its own (non)asymptotic profile likelihood confidence interval setting code. If there was a robust package for this in the python ecosystem, flamedisx could specialize to just computing differential rates for LXe TPCs.

We are currently sprinting to a proof-of-concept paper of our method, so it could be a while before we can give this a shot (as you probably figured out from our slow response time!). As a first step, we would probably still do what @pelssers mentioned above, i.e. add zfit as an additional "minimizer" while keeping our existing interfaces, just so we can test everything. Once this works we can integrate things better.

@jonas-eschle
Copy link
Author

I think this is a great idea: there's no need for every experiment to develop, debug and maintain its own (non)asymptotic profile likelihood confidence interval setting code. If there was a robust package for this in the python ecosystem, flamedisx could specialize to just computing differential rates for LXe TPCs.

This! Exactly that's the whole idea behind zfit: providing a stable package with a reliable workflow so that other packages can specialize on specific implementations and this inference part can be shared. Benefiting in code reduction, common bug finding etcetcetc.

Because, I fully agree, this stable package is ("was") missing, at least when we started two years ago.

We are currently sprinting to a proof-of-concept paper of our method

Of course, I understand, I see it as a long-term goal, step-by-step.
As I tried out already with @pelssers, using the zfit minimizer should already work.

Just let me know when you get things out and wanna give this a try.

@pelssers
Copy link
Member

I've started the zfit-inference branch with the first (very) small steps towards adding zfit. It currently uses zfit to minimize the likelihood for bestfit using zfit.loss.SimpleLoss with zfit.minimize.Minuit. Some changes to the likelihood were needed as zfit wraps the function in a tf.function which we only used for our inner differential rate calculations. The code runs and passes a basic test, I haven't looked at output values yet.

@jonas-eschle
Copy link
Author

That looks good, good to see that! I've seen you finished your paper, congrats.

Yes, I think it could be good to have an extended discussion, if you are interested, on the whole structure of the project and different design decisions, I am very keen to learn about different choices and the advantages - or disadvantages - of them, see what we may can provide from zfit side, and also to better understand in which direction to develop zfit in order to grasp the generality of model fitting with a clean design. We can schedule by e-mail: Jonas.Eschle@cern.ch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants