[Feature request] New classmethod for ModelIsotherm?

[tawe141]

@CorySimon @SimonEnsemble what do you think about letting users create a ModelIsotherm directly from parameters instead of fitting them? This is useful when the raw isotherm data isn't presented in the literature, but authors report the parameters of the isotherms instead. With your permission, I'd like to make a PR to add this feature.

[SimonEnsemble]

yes, this is a good addition. I have a hack for doing that here:

https://gist.github.com/SimonEnsemble/a58ecbe7bc58f4afa817e36bf458be4a

what is your plan for adding this feature? should be in isotherms.py. maybe, in ModelIsotherm, __init__ also have a keyword argument params=None where, if you pass this a dictionary containing params pertaining to the model, it forgoes the fitting procedure? but then it would still require a df. don't see a beautiful way to do this. do you?

[tawe141]

My original plan was to do something like

class ModelIsotherm:
    def __init__(...):
        ...
    @classmethod
    def from_params(params: dict):
        ...
        return ModelIsotherm(...)

But you're right, that would basically be your gist, and I bet there's a prettier way to do it. What about letting df default to None? So if df=None and params are provided, ModelIsotherm would take those as-is. If df=None and params=None, throw an error. Otherwise, it will retain its current behavior.

[SimonEnsemble]

[the @classmethod is new to me. neat.]

your proposal to make df default to None seems most elegant, but that changes the user-interface right? then we'd need to do ModelIsotherm(df=) every time to get the current behavior. might be annoying to users plus quite a lot of work to change the docs, tests etc.

so I'm actually now liking the retrofitting with the @classmethod, since it doesn't change the behavior. while my gist is ugly inside the code, at least from the perspective of the user it is a change they will not notice!

what do you think?

also should error out if (i) any of the params for the model passed are missing, or if it has extra params not in that model.

[tawe141]

I think the user interface should stay the same. You can still specify positional arguments in the same way, it's just that now there's a default value to df. In any case, unit tests should tell us if we're wrong :)

[SimonEnsemble]

oh, right, been a while since I've programmed intensely in Python. I like the way Julia specifies optional and keyword arguments when writing functions. 😄

great, then a PR is welcome by making df=None as you described, as opposed to the @classmethod which is more awkward.

we should mention it in the docs too, either in the FAQ or when discussing ModelIsotherms.

I don't have a test suite set up for this package like I would for a project now (> 5 yrs later!), but I should set one up one day.