Generating plots via plots attribute

[ckoerber]

This was a bit more API changing than expected. But I believe it is much cleaner now.

The FitGUI inside run_server is now set up as follows

    fit_gui = FitGUI(
        fit=fit,
        fit_setup_function=fit_setup_function,
        fit_setup_kwargs=fit_setup_kwargs,
        meta_config=meta_config,
        use_default_content=use_default_content,
    )
    fit_gui.name = name
    fit_gui.get_additional_content = get_additional_content
    fit_gui.plots += additional_plots

I.e., only variables needed on initialization are present on __init__ and things which can be updated on runtime (i.e., public methods) can be added as attributes.

A run_server script now has the form

lsqfitgui.run_server(
    fit_setup_function=generate_fit,
    fit_setup_kwargs={"n_exp": 4},
    meta_config=[
        {"name": "n_exp", "type": "number", "min": 1, "max": 10, "step": 1}
    ],
    additional_plots=[
        {
            "name": "Effective mass",
            "fcn": eff_mass,
            "x-data": x,
            "y-data": y_eff_mass,
        },
        {
            "name": "Effective mass wf",
            "fcn": eff_wf,
            "x-data": x,
            "y-data": y_eff_wf,
        },
    ],
)

In principle, the plot["fcn"] should be of the form

def fcn(fit, **kwargs):
    return go.Figure(...)

and the -data kwargs in the additional plot list are added on top of the fcn plot (corresponding to your transformed data).

Inspired by your decorator approach, I have added the script wrap_plot_function which allows the following API

from lsqfitgui.plot.uncertainty import wrap_plot_function

@wrap_plot_function(kind="band", opacity=0.5, name="fit")
def eff_mass(x, p):
    return np.log(fcn(x, p) / fcn(x + 1, p))

(The wrapper kwargs now address plotting details).

Let me know if this works for your case, update things, and as soon as this is fine, we can merge both PRs.

[ckoerber]

Btw. I use python black as a formatter -- so not every change has an actual meaning...

[millernb]

I think it would be more user-friendly if we removed the wrapper (or at least made it optional). See the comment regarding content.py:get_figures for a workaround.

[millernb]

The eff_mass plot doesn't seem to work without this wrapper, but it would probably be better if the wrapper were optional -- by default, the function should be plotted with a band, eg.

[ckoerber]

I probably should have provided more description here.

The general notion is that plot functions take a fit object as inputs to create a plot. This is inline with the remaining API for, e.g., the callbacks previous plot functions. I believe, that this is the most flexible way to pass in all the dynamic (as in before and after the fit) information and simplifies the creation of complex plots as mentioned in #15.

This wrapper just simplifies the API such that it extracts the x and p values from the fit.

I guess, one way to make it work is provide a different key to the plots dictionary to say that the input is rather x and p than fit; but to me, this is more a documentation issue.

[millernb]

Excluding x-data from prevents y-data from appearing -- should we default to using fit.x? On the other hand, if someone decides to use meta_config to vary x, this default could lead to issues, so maybe it's okay as is.

[ckoerber]

Yes, we should probably, in the long run, think about more possibilities here. At best, when populating the plots-attribute of the GUI, we should also have runtime checks which warn about possible issues.

It certainly needs documentation.

I am more a fan of raising explicit warnings than doing a lot inference on how a plot should be created.
I.e., if a user provides a static y-data array, so by my understanding of plots, also x-data must be static (modulo meta configs which may change both). In this sense the API should rather warn: "you have added y-data without x-data. Do something!" instead of trying to see if fit.x may also resolve this issue. As you point out, such scenarios may cause it to work for one case but maybe not another wich makes it hard to fix.

[millernb]

I don't think this method is necessary, and it only gets referenced once (well, technically twice, if you include DEFAULT_PLOTS). I think it would be better to replace plot_fit(fit) with plot_gvar(fit.x, fit.fcn(fit.x, fit.p))

[ckoerber]

Arguably, that is almost the entire content of this function 😅

As it calls plot, plot_gvar twice (once for the fit, another time for the data), I just found it more convenient to have it in one function instead of one lambda. It might be possible to do a

{"name": "Fit", "fcn": wrap_plot_fcn(kind="band")(fit.fcn), "x-data": fit.x, "y-data": fit.y}

However, if we were to change the x and y data of a dynamic fit = generate_fit(**meta_config), the wrapped function approach would need some custom hacking while the fit based approach can already deal with it (see also my above comment #16 (comment)).

[millernb]

Here's a modification that uses the additional plot functions directly without applying the wrappers; also this removes the need for plot_fit.

fcn = data.get("fcn")
x, y = data.get("x-data"), data.get("y-data")
if data.get("name") == "Fit":
    fcn = fit.fcn
    x, y, = fit.x, fit.y

fig = None

if data.get("name") == "Residuals":
    fig = plot_residuals(fit)
elif fcn is not None:
    x_min, x_max = nanmin(fit.x), nanmax(fit.x)
    fig = plot_gvar(
        linspace(x_min, x_max), 
        fcn(linspace(x_min, x_max), fit.p), 
        kind='band', 
        name=data.get("name"), 
        **kwargs
    )

# ... code for plotting x, y data (below) unchanged

Additionally, using linspace here makes the bands finer (like they were before).

[ckoerber]

Answering this and the next comment in this comment.

I am not a big fan of having internal behavior depend on too many if statements.

The reason why I like the approach with the functions taking fits as arguments is that it is the same for all of them. I.e., you specify their name and the function that does the work. I.e., the plot panel gets a "standard".

Someone who wants to figure out how to add new plots looks at the DEFAULT_PLOTS, sees there needs to be a name and a function. Can look up the body of the function and customize it to get an individual plot.

If we remove this API and make it an internal logic based on names, it is tough to see how it generally works and may one has to understand more lines of code.

[millernb]

I understand your point, though I'd probably address it the other way -- rather than force the additional functions to take the fit as an input, I'd have plot_fit and plot_residuals take x, p, fcn as an input (maybe an optional fit).

[ckoerber]

I see. I guess that is also another valid approach pointing towards #10 .

For now, I'd prefer to take fit objects, as the other API takes them as well. But since plots need well defined x and y values, it is probably valid and makes less assumptions. Let me take a look.

[ckoerber]

For example, the residuals method needs all four values for x, y, p and fcn. I guess it is possible to hack the y into the kwargs of the plots dicts. But since it may be dynamic (i.e., t-range plots), I believe it is best to have these functions still take the maximal amount of information as inputs; i.e., fits.

Once we come up with a different object, we should revisit this.

[millernb]

We can replace these with

DEFAULT_PLOTS = [
    {"name": "Fit"},
    {"name": "Residuals"},
]

if we use change get_figures as suggested in my other comment.

[millernb]

It might be better to have name="data" (or possibly disable the label entirely). The tab heading already has the name of the function, so this seems a bit redundant. (I'd recommend similarly for the bands.)

[ckoerber]

I agree. The reason I kept it was that it is used for the labels / legend inside the plots. But that is probably to some degree redundant as well, as the tab already knows the name. Probably better to just use the kwargs as if actually needed for some reason.

[ckoerber]

Actually, I just checked, I need a unique name for the bands to properly behave under the plotly legend style (as the mean is a separate plot which should also interact with clicks on the legend)

[millernb]

I think we can remove FitGUI.plot_fcns (line 31), too.

[ckoerber]

Yes, I missed this one.

[millernb]

I think we should probably just eliminate this wrapper, as it makes using lsqfitgui less user-friendly. It would probably be better to just use the kwargs in plots to adjust opacity etc.

[ckoerber]

Thanks again for your comments and suggestions. I generally believe we should keep the wrapper as I would advocate that the default syntax should be using fit objects and not x and p as it allows more flexibility in the plots.
I.e., having access to dynamical priors, posteriors, data and fit functions.

However I do see that there is some inconsistency about what are plot arguments and what are computation arguments in both the plots dicts as well as the wrap_plot_function function wrapper.

Let me try to push an edit here.

[ckoerber]

I have pushed a few changes which slightly modify the keywords passed such that things are a bit more inline.

I.e., this wrapper wraps plot_gvar so the kwargs to this wrapper are the keywords passed to plot_gvar

def plot_gvar(
    x,
    y,
    fig: Optional[go.Figure] = None,
    kind: str = "band",
    add_log_menu: bool = False,
    scatter_kwargs: Optional[Dict] = None, # passed to plotly
):

In more detail

@wrap_plot_gvar(kind="band", scatter_kwargs={"opacity": 0.5, "name": "Fit"})
def eff_wf(x, p):
    return np.exp(np.log(fcn(x, p) / fcn(x + 1, p)) * x) * fcn(x, p)


# The above call does effectively the below code


def plot_eff_wf(fit):
    ff = fcn(fit.x, fit.p)
    return plot_gvar(
        fit.x,
        np.exp(np.log(ff / fcn(fit.x + 1, fit.p)) * fit.x) * ff,
        kind="band",
        scatter_kwargs={"opacity": 0.5, "name": "Fit"},
    )

Effectively are valid functions passed to the plots attribute

additional_plots.append({
    "name": "Effective WF",
    # "fcn": eff_wf, # both would do the same thing
    "fcn": plot_eff_wf,
    "kwargs": {},  # passed to eff_mass function directly
    "static_plot_gvar": {  # calls plot_gvar on the same figure
        "x": x,
        "y": y_eff_wf,
        "kind": "errorbars",
        "scatter_kwargs": {"name": "Data"},
    },
})

See also the basic example for more details.

[ckoerber]

Also, I have pushed an example how one can already use this API to generate stability plots: example/lsqfit_stability_example.py. This is jus a draft though and probably needs some optimizing (so many priors...)

.

[millernb]

I like the scatter_kwargs. I'm still not a huge fan of the wrapper as it changes the behavior of the function (eg, eff_mass(x, p) no longer works once the wrapper is applied).

Regardless, I think we've added a bunch of good functionality here. We should go ahead and merge this branch with main (assuming it's stable).

[millernb]

Also, we should address the plots being a bit "choppy", eg

[ckoerber]

I'm still not a huge fan of the wrapper as it changes the behavior of the function (eg, eff_mass(x, p) no longer works once the wrapper is applied).

Ahh, sorry, I believe I missed/misunderstood this argument before. Indeed, if not properly explained, this can lead to confusions. The only direction I see here would be not including wrappers at all.

Regarding the "coppy" bands. Yes, I've already re-introduced it in the wrapper (but not plot_fit which creates this one). Unfortunately, I could not make it work the same way (linspace x and get the corresponding ys) for plot gvar, as the ys are already computed (I guess one could also interpolate y). But it should be possible to just copy the functionality into the plot_fit method. I'll add it there and merge the new versions.