arviz-devs / arviz-plots

ArviZ modular plotting
https://arviz-plots.readthedocs.io
Apache License 2.0
3 stars 2 forks source link

New module for input verification proposal #83

Closed amaloney closed 1 month ago

amaloney commented 3 months ago

There is replicated code amongst a couple different plots. As an example, when credible intervals kinds are checked, we duplicate the same code in two different places.

https://github.com/arviz-devs/arviz-plots/blob/a1e32a5dc565ec4ffad317ee106cf9f1f3592bda/src/arviz_plots/plots/forestplot.py#L168-L169

https://github.com/arviz-devs/arviz-plots/blob/a1e32a5dc565ec4ffad317ee106cf9f1f3592bda/src/arviz_plots/plots/distplot.py#L151-L152

I'd like to propose a new module in plots called verify, where we can add methods like the below example.

def credible_interval_kind(ci_kind: str | None) -> str:
    if ci_kind not in ("hdi", "eti", None):
        raise ValueError("`ci_kind` must be either 'hdi' or 'eti'.")
    if ci_kind is None:
        ci_kind = rcParams["stats.ci_kind"]
    return ci_kind

This way in the plot code we can do the following.

from arviz_plots.plots import verify

def plot_forest(...):
    ...
    ci_kind = verify.credible_interval_kind(ci_kind=ci_kind)
    ...

We can extend this to other checks, e.g. sample_dims. We could also extend it further and have all "verification" checks for a plotting method wrapped up into a single method like verify.forestplot_args() or the equivalent. The goal would be to move the if statements at the beginning of each of the *plot.py methods to their own module where we could even add checks for plot_kwargs or pc_kwargs if needed in the future.

@OriolAbril if this sounds reasonable, you can assign the task to me.

OriolAbril commented 3 months ago

That would be great, thanks! It would also make https://github.com/arviz-devs/arviz-plots/issues/66#issuecomment-2212494391 quite straightforward to implement

amaloney commented 3 months ago

Great, I cannot assign myself the ticket. I will if you are okay giving me pull access to the repo. Otherwise I can clone etc...

OriolAbril commented 3 months ago

You should have write permission to this repo as ArviZ core contributor. Probably means I messednup when setting it up. I'll check and fix it later today

OriolAbril commented 3 months ago

Should be fixed now

OriolAbril commented 2 months ago

I added something like this to https://github.com/arviz-devs/arviz-stats/pull/22 but now I am thinking the bulk of that should probably go in arviz-base instead, with each submodule adding extra functions that can't be in arviz-base. So you think it would make things very confusing to import validators from different places?

I guess in the validate.py of arviz-stats we could from arviz_base.validate import * so within arviz_stats we import all validators from the same place

amaloney commented 2 months ago

Hmm, maybe it would be confusing to import from different places.

In general the from X import * pattern is frowned upon, so my opinion would be to not do that.

Let me think about an architecture for this. I've seen patterns that may solve this issue, but will need a minute to think it through. I'll create a discussion topic so we can figure out different ideas on how to solve this.

amaloney commented 1 month ago

@OriolAbril I am closing this in favor of creating one in the arviz_base repo, since I think that is where we should "start" adding a verification module. I will link to this discussion there.