search

OxIonics / ionics_fits

Small python fitting library with an emphasis on Atomic Molecular and Optical Physics
Apache License 2.0
1 stars 0 forks source link

Refactoring of FitModel and some other changes #10

Closed mbirtwell closed 2 years ago

mbirtwell commented 2 years ago

FitModel parameters are now an instance attribute. This allows two modes of using FitModel.

This first is by overriding _func this allows you to have a better typed experience. The FitParameter metadata is attached as annotations to the arguments of _func and automatically extracted from there.

The second mode is to for dynamic parameter sets. The parameters are passed explicitly to the FitModel constructor and func is overridden.

The renaming utility is re-cast as a FitModel with dynamic parameters and to me at least feels less magic now than it did before because it can do all the work as regular methods called a wrapped FitModel object rather than having to do dynamic class creation.

In addition to updating the tests to instantiate FitModel objects there are a couple of tweaks to the imports to make them work with my tooling.

mbirtwell commented 2 years ago

I got a bit nerd sniped by this. What I was originally trying to do didn't work out. But I think this has worked out quite well. It's just about meaningful to have different instances of the same FitModel class (in the case of Polynomial), so it feels right for everything to be instance methods on it now.

mbirtwell commented 2 years ago

@hartytp It looks like I accidentally merged this just before pushing the changes for your comments above. Sorry about that. I think the best thing is probably to just merge them in to your branch also. I'll do that now.

mbirtwell commented 2 years ago

@hartytp It looks like I accidentally merged this just before pushing the changes for your comments above. Sorry about that. I think the best thing is probably to just merge them in to your branch also. I'll do that now.

Oh I think I know what happened. I had the wrong upstream configuration on my local branch and I didn't spot that when I pushed. And because it was a valid fast-forward it just went with it. Ooops again sorry. But hopefully this where we were going to end up soon anyway. We're just missing the merge commit.

hartytp commented 2 years ago

no problem. Your changes look really good, thanks again for this.