Naming convention (binsize -> bin_size, etc.)

[dizcza]

Update Sep 08, 2020

The voting session is closed: any additional votes will be ignored. If you find the current naming convention inconvenient, open an issue or discuss it with us on Neuralensemble mailing list or gitter chat.

---

IMPORTANT. The old arguments are deprecated but not removed. Even though it looks like they are removed from the code, the old signature will still work - it just throws a warning, which can become a hard error in 2 or 3 releases since then, if we want.

Below is a compiled list, made by elephant doc team with ❤️

Deprecated arguments mapping

Elephant-wise

• binsize --> bin_size
• num_bins --> n_bins in all modules except GPFA transform_info dict that has the key 'num_bins'; commit 0408896
  
• binned_st[s] --> binned_spiketrain because binned_spiketrains, as spiketrains, would imply a list of BinnedSpikeTrain objects
• fs -> sampling_frequency. fs is used in 4 elephant functions: butter, wavelet_transform, welch_psd, and welch_cohere.
  
  
• _freq -> _frequency? freq is used 4 times: butter, welch_psd, welch_coherence, and GPFA
  
  
• a pair of similar signals, spiketrains, and other variables will have suffixes _i and _j. Below is a list of affected functions.

elephant/asset.py:    def __init__(self, spiketrains, spiketrains_y=None, bin_size=3 * pq.ms,
elephant/asset.py:                 t_start_x=None, t_start_y=None, t_stop_x=None, t_stop_y=None
elephant/spike_train_correlation.py:    _CrossCorrHist.__init__(self, binned_spiketrain1, binned_spiketrain2, window):
elephant/spike_train_correlation.py:    def get_valid_lags(binned_spiketrain1, binned_spiketrain2):
elephant/spike_train_correlation.py:    def spike_time_tiling_coefficient(spiketrain_1, spiketrain_2, dt=0.005 * pq.s):
elephant/spike_train_correlation.py:    def run_P(spiketrain_1, spiketrain_2):
elephant/spike_train_correlation.py:    def cross_correlation_histogram(binned_spiketrain1, binned_spiketrain2,
elephant/spectral.py:def welch_cohere(x, y, n_segments=8, len_segment=None, freq_resolution=None,

Function-specific

• cubic
  • data -> histogram
  • ximax -> max_iterations
• victor_purpura_dist
  • trains --> spiketrains
  • q -> cost_factor; is there a particular reason, a paper, where q is used?
• cv2 and lv v --> time_intervals
• cell_assembly_detection
  • data --> binned_spiketrain
  • maxlag --> max_lag
  • min_occ -> min_occurences
  • same_config_cut -> same_configuration_pruning
• cross_correlation_function
  • ch_pairs --> channel_pairs
  • nlags --> n_lags
  • env -> hilbert_envelope
• lst_input -> input_spiketrains in BinnedSpikeTrain; closes renamed 'lst_input' -> 'spiketrains' in BinnedSpikeTrain class' #311
• hilbert N --> padding (N is passed in scipy.signal.hilbert, which has this argument)
• wavelet_transform
  • nco -> n_cycles
• welch_psd, welch_cohere
  • num_seg -> n_segments
  • len_seg -> len_segment
  • freq_res -> freq_resolution
• welch_cohere() -> welch_coherence(). As a remark, scipy, to my surprise, uses scipy.signal.coherence and not scipy.signal.cohere.
• spike_extraction extr_interval -> interval - the function name already has the key "extraction"
• peak_detection format -> as_array
• single_interaction_process
  • rate_c -> coincidence_rate
  • return_coinc -> return_coincidences
  • n -> n_spiketrains
• JointISI refr_period -> refractory_period
• surrogates surr_method -> method
• sskernel --> optimal_kernel_bandwidth
  • w -> bandwidth
  • tin -> times
• spike_train_surrogates module
  • n --> n_surrogates
• estimate_csd coords -> coordinates
• kernels t -> time
• extract_neo_attrs -> extract_neo_attributes
  • obj -> neo_object
• generate_lfp
  • ele_xx -> x_positions or positions_x
  • xlims -> x_limits or limits_x? Currently, it's x_limits, but then why do we use max_something and not something_max?
  • res -> resolution;
• multiple_filter_test
  • dt -> time_step or step
• empirical_parameters
  • dt -> time_step or step
• victor_purpura_dist -> victor_purpura_distance
  • trains -> spiketrains
  • q -> cost_factor
• van_rossum_dist -> van_rossum_distance
  • trains -> spiketrains
  • tau -> time_constant
• spike_train_timescale
  tau_max -> max_tau
• compound_poisson_process A -> amplitude_distribution
• cross_correlation_histogram cross_corr_coef -> cross_correlation_coefficient
  
  
  
• corrcoef() -> correlation_coefficient()
  
  
  

Not refactored

Because they require special care (ASSET already follows the naming convention):

• SPADE
• Unitary Event Analysis
• GPFA

Also, I didn't pay attention to _protected functions not because they can avoid the naming convention but because I didn't want to spend time refactoring them. Keep in mind that any decent renaming may lead to breaking the logic and thus should be justified.

Won't change

• In homogeneous_gamma_process() function a is the shape and b is the rate of a distribution will not be renamed to avoid a misinterpretation of the rate of the outcome spiketrain, which is equal to b/a and not b.
• In surrogates() function dt won't be changed because it means different things depending on the surrogate method choice.
• In multiple_filter_test() and empirical_parameters() functions t_final won't be changed to t_stop because it serves a different purpose.
• nfft became a standard over the years, even though it does not comply with our convention, which renders this variable as n_fft.
  
  

[pep8speaks]

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

• In the file elephant/test/make_spike_extraction_test_data.py:

Line 14:5: E741 ambiguous variable name 'I'

• In the file elephant/test/test_signal_processing.py:

Line 1073:80: E501 line too long (81 > 79 characters)

Comment last updated at 2020-08-07 11:56:20 UTC

[coveralls]

Coverage decreased (-0.2%) to 88.983% when pulling 4d19722 on INM-6:naming-convention/bin_size into 9007517 on NeuralEnsemble:master.

[kohlerca]

Regarding cross_coef/cross_corr_coef in spike_train_correlation:

The parameter in cross_correlation_histogram function is passed to the protected class _CrossCorrHist. Therefore, according to the conventions of just using corr as an abbreviation for correlation, cross_corr_coefficient will make the information clearer. The fact that it is passed internally, I don't see as an annoyance of a larger parameter name.

Moreover, in _CrossCorrHist, the class method should be cross_correlation_coefficient to keep the standard as in spike_time_tiling_coefficient, and to align to what we discussed regarding keeping the Elephant function names without abbreviations. The latter also applies to function corrcoef. Although NumPy has this function, in Elephant we are talking about an implementation that works with elephant.BinnedSpikeTrain inputs. I don't see any benefit in shadowing NumPy.

Finally, in the class _CrossCorrHist, the current cross_corr_coef function takes an argument named only cross_corr. It should also define which is the input, as cross_corr* is a prefix for histogram or matrices. Therefore, something in the direction of cross_corr_array or cross_corr_matrix will make the code clear.

[kohlerca]

Regarding _x and _y or _1 and _2 in welch_cohere and ASSET:

I don't think they can be analyzed together. The idea of _1 and _2 is for the functions where the parameter order does not matter, which is the case of welch_cohere.

In ASSET, the spiketrains_y will actually reflect in the interpretation of the matrices axes. Therefore, _x and _y is meaningful with respect to the method output. ASSET is tough because you don't need both parameters, but I think it is better to name the first (non-optional) as _x and the optional as _y.

[mdenker]

@kohlerca Regarding _x and _y or _1 and _2 in welch_cohere and ASSET.

Also in welch_cohere, the order of parameters makes a big differene when interpreting the phase_lag return value, since the order defines the reference and the referenced, so to say. In this way, I think the situation is very similar to ASSET. Now, I think the main reason we dislike 1 and 2 is because it looks terrible in the variable name, and I think we dislike x and y because it is commonly associated to a position. How about a compromise and using i and j? This is commonly used to identify this type of relationship...

[kohlerca]

@mdenker How about a compromise and using i and j? This is commonly used to identify this type of relationship...

I guess this makes total sense and it is better indeed than either of the previous options.

[dizcza]

@mdenker How about a compromise and using i and j? This is commonly used to identify this type of relationship...

I guess this makes total sense and it is better indeed than either of the previous options.

Added _i and _j option in the poll.