Do something sensible when running Minuit.hesse() turns valid minimum into invalid minimum

[HDembinski]

The following is a concern when using strategy 0 or 1 (default).

In most cases, Minuit.migrad calls MnHesse automatically at the end of the minimization, but if the correlations between parameters are small, then it does not.

In the former case, calling Minuit.hesse() after Minuit.migrad() does not do anything. In the latter case, calling Minuit.hesse() can turn a valid minimum into an invalid minimum, because the EDM value depends on the Hessian matrix, which is updated by Minuit.hesse(). This can turn a EDM value below threshold into a value above threshold and Minuit then reports that the fit has failed.

There are two options for what to do here:

• Raise a IMinuitWarning when a valid minimum is turned invalid by running Minuit.hesse(). This gives users a chance to understand the issue and react to it accordingly. It is the responsibility of users to check Minuit.valid after Minuit.hesse() and run Minuit.migrad() again if Minuit.valid is False.
• Minuit.hesse() calls Minuit.migrad() automatically if Minuit.valid is True before and False after calling MnHesse.

The second option is the most convenient for the casual user, but may irritate users that expect Minuit do to exactly what it is told and not more (people who use Minuit as a backend).

[HDembinski]

@matthewfeickert @jpivarski @henryiii Do you have thoughts on this?

[jpivarski]

An argument to the hesse function: on_new_minimum="migrad", "warning", "error"?

[HDembinski]

@jpivarski This goes along the solution you often used in uproot, right? I think it is a good way to change behavior of the API in a soft way with a deprecation period. But I still need to decide what should be the new default.

[jpivarski]

Some kinds of changes can have a deprecation period and others can't.

Changing the name of a function, class, or method is easy: you add the new function/class/method and put a warning on the old one saying that it's going away and users should switch to the new one. An example of that is NumPy's tostring method: they added tobytes and a warning on tostring. Eventually, tostring went away. The warning should say how to change and in what version number the old one will go away. It would be more useful to also have a date, but that's usually hard because extenuating circumstances can affect the date at which a version gets released.

It's much harder to change the default value of an argument to a function or method, which is what you want to do here. Suppose that you want to introduce an argument on_new_minimum="nothing", "migrad", "warning", or "error". The argument does not exist yet, but the current behavior corresponds to one of the argument's options, on_new_minimum="nothing". If you just want to add the new argument and its default will be equivalent to the current behavior, then no deprecation cycle is needed: you can just add it. But if you want the default to be, say, on_new_minimum="migrad", then there will have to be a period of time during which users are warned that the behavior will change. (Actually, one of those options, "warning", is equivalent to "nothing" from a deprecation-cycle point of view, since emitting a warning is what you'd do to tell users that a change is coming: it can't be considered a change in itself! If no one looks at the terminal, only the final results, it's not functionally different from "nothing".)

We had to do this once: among the API changes in Awkward Array (v1), "arrayset → buffers" was like "tostring → tobytes" in that we introduced new behavior by putting the new behavior in a function with a new name and deprecating the old one, and "ak.fill_none's axis → -1" was like "on_new_minimum → "migrad". Originally, ak.fill_none did not take an axis argument, and its behavior was equivalent to... I might not be remembering exactly, but I think it wasn't like any particular axis value; I think it descended until it found missing values and then filled them, without regard to what depth they were at. (Good reason to change it; that's terrible.) For a period of time, we (@agoose77) made it such that if you didn't pass any axis option, it would continue the old behavior but emit a warning, and if you pass an axis, it would use that axis. Internally, this meant two implementations of the function and a semaphore object,

unset = object()

def fill_none(array, axis=unset):
    if axis is unset:
        ...

to be able to distinguish between existence and non-existence of the axis argument in the call. (Handling *args and **kwargs would work as well here, but isn't automatically documented as nicely.) The warning included the nature of the coming change, the version number when it would be applied, and an estimate of the date (which turned out to be entirely wrong). The only way to shut the warning up was to explicitly set an axis, and presumably, that's what a lot of users did.

That said, not all changes in behavior require deprecation cycles. If the old behavior can be considered a bug, it can be fixed right away (unless the library has some very hard consistency guarantees, such as one that would be used in life-threatening situations, but that's not what we're talking about). What is a "bug" and what is a "feature" is a subjective thing: it depends on the author's intentions, and authors don't always know their intentions. Like a (Copenhagen) quantum state, some decisions aren't made until the question is asked. A rigorous procedure can't always be applied, and it's legitimate to consider it a judgement call.

In this particular case, what I would do is introduce the argument and make the default behavior equivalent to the old behavior, so that no deprecation cycle is needed, because they're a pain. "Equivalent" can include a warning, so the new behavior can be "warning".

[henryiii]

Before sending edit: Yes, I wrote this before reading the middle of Jim's reply and reading that's exactly what he did. So I guess I actually had nothing to add. 🤦

I don't have much to add, just can mention that there is a procedure for nicely changing the default for an argument. You can require users provide the argument always and produce a warning if they don't. That way you can eventually safely change the default, since users should be all explicitly specifying the option anyway (assuming that they update and see the warning during the period).

This can be seen in some libraries, but probably the highest-profile example: Python 3.10+ will produce a warning if encoding= is missing on any use¹ of open (being Python, they have a global flag -X warn_default_encoding to enable it, can't do that for a library! But same idea). That's actually a similar situation - open() has a confusing encoding default, they'd really like to make it "utf-8" and make "native" an explicit choice instead of default, but they are stuck. I don't know if they will actually make the switch, but forcing explicitness is a good intermediate step for something like this. (People are terrible at turning on optional warnings, as the recent click 8.1 demonstrated, so I don't think this will be enough to make it possible to switch, but it's something they are toying with).

Doesn't mean you want/need/should change it - "warning" might be a fine default, just pointing out that there is a procedure you can follow and still be "nice" if you do want a non-compatible default.

Footnotes

1. Opening in text mode, that is, there's no encoding for a binary mode, obviously. ↩

[HDembinski]

I can change the API at any time by adding a warning. This is a non-breaking change and does not need a deprecation period. Changing the API so that Migrad is called after Hesse broke the fit can be considered "a fix", it is technically a breaking change on the user side, but one that rectifies a problem. We usually make such changes without a deprecation period, too.

Anyway, I didn't want to discuss how to change the default for an argument, but what kind of behavior would be the best default in this case in the long-term.

I was asking you not as Python programmers but as people who are familiar with Minuit.

[HDembinski]

I don't have much to add, just can mention that there is a procedure for nicely changing the default for an argument. You can require users provide the argument always and produce a warning if they don't. That way you can eventually safely change the default, since users should be all explicitly specifying the option anyway (assuming that they update and see the warning during the period).

Yes, I am aware of that technique and would use it where appropriate. I was not asking about how to make the transition, I know how to do that. I was asking what the new default should be.

Jim suggested "warning", which is the safe option, but I am not sure whether this solution is the most beneficial for the average user.

To clarify further, I am not interested in what is the easiest solution now, I consider what is the best solution long-term. And that goes into fundamentals of what iminuit should be.

iminuit is already not just a dumb wrapper around Minuit2. Minuit.migrad calls MnMigrad several times by default, because that improves the convergence rate. It is a silly hack that I inherited from the previous maintainers. I originally wanted to remove it, but then I got complains that iminuit's convergence rate was reduced (from pyhf, I think). So I restored that hacky feature. Since we already try to improve the user experience beyond what calling raw MnMigrad would give you, it is just consistent to also call Migrad after Hesse. However, that has potential unwanted side-effects, too.

I guess it is difficult to see all the implications unless you are the long-time maintainer of iminuit.

[jpivarski]

Anyway, I didn't want to discuss how to change the default for an argument, but what kind of behavior would be the best default in this case in the long-term.

Oh, sorry I misunderstood. In that case, I would expect a new minimum found in HESSE to invoke another MIGRAD. I think MINOS already does that internally: I remember there being a big ASCII arrow in the log output when that happened.

It's good for this to be parameterized, because there are reasons to intentionally call HESSE on non-minimal points in the space, if what you're trying to do is get a second-derivative map of it. That's not how Minuit is usually used, but it wouldn't be a bad use.