FlamTeam / flamedisx

Fast likelihood analysis in more dimensions for xenon TPCs
https://flamedisx.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
15 stars 13 forks source link

zfit loss and minimizer #41

Open jonas-eschle opened 5 years ago

jonas-eschle commented 5 years ago

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:

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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

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 commented 4 years ago

(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 commented 4 years ago

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

JelleAalbers commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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