Skip to content
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] Added ASSET class initialization parameter to define the binning rounding error tolerance #585

Merged

Conversation

kohlerca
Copy link
Collaborator

Currently, the ASSET class performs the binning of the spiketrains using the default behavior of the BinnedSpikeTrain class in conversion.

Therefore, when spike times are closer to the right bin edge than a defined tolerance value, the BinnedSpikeTrain class shifts the spike to the next bin. This behavior can be controlled by the tolerance parameter of BinnedSpikeTrain.

This PR adds the new parameter bin_tolerance to the ASSET class, in order to allow the user to define the desired tolerance (and hence BinnedSpikeTrain behavior).

The parameter can take a string 'default' to use the default behavior of BinnedSpikeTrain (and which corresponds to the current status of ASSET before this PR), or a float value or None. In the latter two cases, these values are forwarded to the binning class, and will result in either shifting or not shifting the spikes, respectively.

@Moritz-Alexander-Kern Moritz-Alexander-Kern changed the title Added ASSET class initialization parameter to define the binning rounding error tolerance [ENH] Added ASSET class initialization parameter to define the binning rounding error tolerance Jul 25, 2023
@Moritz-Alexander-Kern Moritz-Alexander-Kern added the enhancement Editing an existing module, improving something label Jul 25, 2023
@coveralls
Copy link
Collaborator

coveralls commented Jul 25, 2023

Coverage Status

coverage: 86.979% (+0.002%) from 86.977% when pulling f61cd1e on INM-6:fix/asset_bin_tolerance into 2bd871a on NeuralEnsemble:master.

Copy link
Member

@Moritz-Alexander-Kern Moritz-Alexander-Kern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this nice enhancement of ASSET @kohlerca 🎉

elephant/asset/asset.py Outdated Show resolved Hide resolved
elephant/asset/asset.py Outdated Show resolved Hide resolved
@kohlerca
Copy link
Collaborator Author

Thanks for the review, @Moritz-Alexander-Kern. I made the requested changes and also modified the unit tests slightly so that the binning of each of the two input lists is checked (same spiketrains, but reverse order).

Copy link
Member

@Moritz-Alexander-Kern Moritz-Alexander-Kern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks implementing the requested changes so promptly @kohlerca .
Approved from my side. ✔️

@Moritz-Alexander-Kern Moritz-Alexander-Kern added this to the v0.13.1 milestone Jul 26, 2023
@Moritz-Alexander-Kern Moritz-Alexander-Kern modified the milestones: v0.13.1, v0.14.0 Sep 12, 2023
Copy link
Member

@mdenker mdenker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all good to me, unit tests are also understandable.

@Moritz-Alexander-Kern Moritz-Alexander-Kern merged commit 4009abf into NeuralEnsemble:master Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Editing an existing module, improving something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants