OPT/UnitaryEventAnalysis: optimize calculation/rounding of 'n_bins' in 'jointJ_window_analysis()' function

[ojoenlanuca]

This (optional) pull request presents an alternative and numerically more stable way to calculate the number of bins 'n_bins' within the 'jointJ_window_analysis()' function in 'elephant/unitary_event_analysis.py' module. It uses the 'round_binning_errors()' function from 'elephant/utils.py' module to perform the calculation/rounding of 'n_bins'. In that way, rounding error are avoided if unusual time units like 'ns' or 'ks' are used.

This rounding procedure was originally introduced in the inner function 'get_n_bins()' of the private method '_resolve_input_parameters()' of the 'BinnedSpikeTrain' class in the 'elephant/conversion.py' module.

One can consider this pull request also as starting point of a discussion about which physical units are explicitly/implicitly expected by 'elephant' functions and which are excluded/not supported.

[Moritz-Alexander-Kern]

Hey @ojoenlanuca , thanks for contributing this enhancement. I agree lets discuss about physical units for elephant.

[Moritz-Alexander-Kern]

~~In order to use a unified way of calculating the number of bins in elephant, we could put the suggested approach into a separate function in utils.py. Example:~~

def calculate_n_bins(t_start, t_stop, bin_size):
     ...
     return n_bins

The calculation of n_bins is done in a similar way in statistics.py, instantaneous_rate, see:

https://github.com/NeuralEnsemble/elephant/blob/0df45812b93229b3186c061cb84ec29fd380e09c/elephant/statistics.py#L864

This would avoid various implementations of the same functionality across elephant.

Additionally is_time_quantity from elephant.utils could be used here to check if t_stop, t_start, bin_size are time quantities.

If you have an idea for a unit-test, which could test for the rounding error that is now avoided with the new implementation, this would be a really welcome addition to this PR @ojoenlanuca .

[pep8speaks]

Hello @ojoenlanuca! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2022-08-09 13:33:20 UTC

[Moritz-Alexander-Kern]

• added unit-tests,
• fixed formatting,
• changed warning to TypeError

[coveralls]

Coverage: 87.455% (+0.2%) from 87.21% when pulling 97e04fffb57e48d14850117352d6b7dda5bad1b0 on INM-6:optimization/unitary_events_analysis__n_bins_calculation into df8e96212e121d5931bab6c9ae82e059c91ec2ad on NeuralEnsemble:master.

[Moritz-Alexander-Kern]

Outdated, but can be reopened at any point.