-
Notifications
You must be signed in to change notification settings - Fork 98
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
[ENH] - Add SpectralTimeModel
and SpectralTimeEventModel
#283
Conversation
@benderas7 - a big belated thank you for your deep dive here, and apologies it took me so long to get back to working on this update again! Your comments were super helpful, and helped fix some specific things, as opening up some new tests / ideas that also lead to further tweaks and improvements. As a quick overview of the recent specific changes based on your comments:
And a couple more comments:
|
Okay - I've made a series of relatively minor updates here, though nothing too major. Instead of trying to tag people back in for a review of this rather massive PR, I'm going to merge this in, tag a new release candidate, and we can check and tweak from there. To be clear - it's not that I don't want further checks / suggestions, etc, precisely the opposite - I think the better way to try and check this update from here is to merge it, get people trying it, and then track any quirks / work on improvements from there! |
Overview
This PR adds new model objects to manage applying the spectral model across time (spectrograms), and across events whereby each event is across time (multiple spectrograms). It also includes associated additions / updates / re-organizations to help with the extension to explicitly supporting applying the spectral model over time windows, and code updates related to better supporting the updated strategy of having more / different model objects for different use cases.
Overall, this PR adds new model objects
SpectralTimeModel
, andSpectralTimeEventModel
, which closely mirror our previous objects. Notably,SpectralTimeModel
is really a special case ofSpectralGroupModel
to take spectrograms, andSpectralTimeEventModel
extends this functionality to support analyses across multiple spectrograms. Practical updates to the new objects include updates to things like the plots / reports in order to best describe / work with the parameters given the new context of estimations (eg. plotting parameters over windows).Perhaps the most notable update (to be able to do this) is to managing peak parameters, for which we have to choose a strategy for organizing peaks across models. To do so, here we follow the logic started / developed for exporting model results to dataframes (#196) - in this approach the main / suggested approach is to provide a "Bands" object definition, which is then used to organize parameters by defined bands. We should perhaps keep in mind / consider the pros / cons of this in terms of things like how to do so we select a single dominant peak (which may not properly reflect bandwidth, if there are overlapping peaks, etc). Note that while a new
time_results
(orevent_time_results
) attribute is added and used here, the fullgroup_results
(orevent_group_results
) are maintained, such that no information is lost / discarded (just sub-selected from).Examples
Overview examples of the two new objects:
SpectralTimeModel
Quick version example of
SpectralTimeModel
- given a spectrogram of PSDs over time windows, this can be parameterized as such:This would give the following output:
SpectralTimeEventModel
Quick version example of
SpectralTimeEVentModel
- given a list of spectrograms of PSDs over time windows across a set of events (assuming the same set of windows across events), this can be parameterized as such:This would give the following output:
Full example code that can be run
For testing & exploration, this PR includes associated updates, such as adding a
sim_spectrogram
function, which can be used to explore this functionality.Example self-sufficient code to run these examples:
Additional Notes
Additional notes & comments:
get_params
,get_results
,get_model
, etc)Reviewing
In terms of reviewing - I've tagged core development related people (@rdgao, @ryanhammonds, and @voytek) for explicit review - but I would suggest that before we do the deep dive for any nitty-gritty code checks, that a first sweep through for people to have a look, and try it out, and comment any high-level ideas about the general strategy taken here. Once we've done this kind of check, and other people have run the basics and if we decide we like this approach, then we can coordinate for any detailed code review stuff.
As part of this "general check-in" review, it would be great to here from people who are also on the user side of things, for which I'm tagging in @mwprestonjr @benderas7 @dillancellier @sydney-smith @QuirinevEngen (though anyone untagged is more than welcome to also give comments / and no worries at all if people don't have time to try this out right now).
To install and test this new functionality:
Once installed, you can open a notebook (or however you prefer) and start with the example code section above that should run and give you example reports matching what I pasted in. It would also be great if people can try it out on any real data they have that matches these use cases (as I have not done this yet).