-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] - Add explicit {aperiodic, periodic} 'Modes' support #298
base: basemodel
Are you sure you want to change the base?
Conversation
@ryanhammonds & @voytek - tagging you here so you can get an eye on this, to see where I'm trying to move things here, and how this relates to #291. It's not ready for a review in any real way - but thoughts (especially "yay" / "nay" votes on this direction) are appreciated! |
On the surface this looks good. I can't quite tell, but does this construction allow for something like the dual-knee fitting approach we've been playing with, for example? |
@voytek - yeh, the goal is that it extends to this - the Mode definition would include the function and list of parameters, and then the rest of the code would read this definition to dynamically updates things like the expected number of parameters and labels, etc. That part of it isn't built in yet - but I don't currently foresee any major roadblocks to doing it! If this looks good as a first draft / direction - I can keep working to get to some proof of concepts that get a bit further and show support for an actually new / different fit function! |
Cool, this looks like it track's important/relevant metadata for new models. What is the plan for handling parameter guess procedures? I guess that's kinda related to #291. I guess for each parameter of new models model there could be either a hard-coded guess/bounds or new procedures to generate the guess/bounds. |
[MNT] - Add setable alpha level to add_shades
Fixed matplotlib is required
This is the start of exploring and using fit "modes" to define and use
aperiodic_mode
andperiodic_mode
as flexible fit modes that not only define and support a way to add new fit modes to the model, but also set up the infrastructure to allow for passing in fit functions."Fit Mode" idea
The notable update is (currently) in
specparam.objs.modes
, where we can see a current working definition of what is needed to define in order to define a fit mode. Once we define this, it takes relatively minor updates across the rest of the files to get most functionality* running for our current fit functions, including everything working for the core model fit. (* Note that currently some tests are turned off (all the IO stuff, and a couple object manipulations). There should be no fundamental issue here, it'll just take some updates to propagate certain information in the new way). For this kind of standard use, this is another change that allows for more customization, but shouldn't at all change standard operations.For a brief overview, the base idea is that for each of the {aperiodic, periodic} modes, we define a "Mode" for how we fit this. Specifically, this is basically the fit function itself, but also, we define a set of associated meta-data that describes the parameters and so on. By using this
Mode
information, we don't have to hard-code information about number of parameters / labels, etc, and can read this on the fly from the Mode definition.Passing in fit functions
In addition, this also allows for the basics of passing in a new fit function (note that this is currently a basic proof of concept).
Note that currently, this doesn't allow for passing in any fit function - notably I picked a very simple example which works as it has the same name and number of parameters as our current 'knee' function (one could also currently do this relatively easily by tweaking the internal module code). The simplicity here is just to show the idea, and because the current update is limited in scope (see next section) - but I think this should generalize, and allow for the kind of flexible specifications we are aiming for!
ToDo From Here
Currently, this implementation is very bare bones and just about works - but the bulk of the work to actual get this strategy properly integrated is not yet done (so far, this serves as a high-level strategy check). The core of this is that there is still a lot of hard-coded stuff across the code, and as much as possible this needs to be updated to dynamically read key information from the
Mode
object, allowing for flexibility (across things like accessing parameters, printing results, reports, plots, IO, manipulating objects). I think at least most of this should work, but the details of how much / well this works will probably dictate how far this gets in practice. If it gets far enough, the example approach to pass in novel fit modes from above could / should allow for passing in arbitrary fit functions, and managing the outputs accordingly.If you explore the Mode object, there are some aspects that look further forward, in particular defining the data spacing. The idea is that a particular mode can be defined on a particular spacing (for each of 'freqs' and 'powers'), and so by defining this, and ultimately setting and checking this in relating to the Data object, one can define arbitrary fit functions across arbitrary data representations. When integrated with allowing for arbitrary fit procedures from #291, we get the full level of generalization!
Reviewing
Way too early to review for real, but very open to any strategic / conceptual questions / comments / concerns at this point.
Note also that this PR is directed at the
basemodel
branch of #291 - meaning it relates to and builds upon the updated object organization from there. It's probably not necessary to use that object organization, but in my mind these updates do work together.If you do want to look at the code, the notable new stuff is in
specparam/objs/modes
, and some relevant updates inspecparam/objs/model
. Ignore all changes in the tests folder - this just turns off some tests that currently fail as more stuff needs to be updated to get this to fully work.