[DOC] Clean up docstring formatting #283
Conversation
| Note: In mne-connectivity, we associate the word | ||
| "spectral" with time-frequency. Reference to | ||
| eigenvalue structure is not captured in this mixin. | ||
| Note: In mne-connectivity, we associate the word "spectral" with time-frequency. | ||
| Reference to eigenvalue structure is not captured in this mixin. |
| If these are computed over a frequency band, it will | ||
| be the median frequency of the frequency band. | ||
| If these are computed over a frequency band, it will be the median frequency of | ||
| the frequency band. |
| combine : 'mean' | 'median' | callable | ||
| How to combine correlation estimates across epochs. | ||
| Default is 'mean'. If callable, it must accept one | ||
| positional input. For example:: | ||
| combine : ``'mean'`` | ``'median'`` | callable | ||
| How to combine correlation estimates across epochs. Default is ``'mean'``. | ||
| If callable, it must accept one positional input. For example:: |
There was a problem hiding this comment.
Line length and use literals
mne_connectivity/base.py
Outdated
| conn : instance of Connectivity | ||
| conn : instance of Connectivity | SpectralConnectivity | TemporalConnectivity | SpectroTemporalConnectivity |
There was a problem hiding this comment.
The return matches the non-epoched class type of whatever class is being combined. Is specifying that it could be more than just Connectivity too verbose?
There was a problem hiding this comment.
for the return types, verbosity is fine if it's needed to be accurate. If there's a succinct way to sum it up, that can go in the textual description, so in addition to "The combined connectivity data structure" it could say something like "type will reflect the input data type (plain, spectral, temporal, spectrotemporal) but without the epoch dimension."
There was a problem hiding this comment.
Okay. In that case I might return to just Connectivity and add context in the description.
There was a problem hiding this comment.
Perhaps:
mne-connectivity/mne_connectivity/base.py
Lines 145 to 149 in 1f9484b
| data : array | ||
| Epoched or continuous data set. Has shape | ||
| (n_epochs, n_signals, n_times) or (n_signals, n_times). | ||
| data : array, shape ([n_epochs,] n_signals, n_times) | ||
| Epoched or continuous data set. |
There was a problem hiding this comment.
Line length and shape alongside type
| For multivariate methods, this is handled differently. If "indices" is | ||
| None, connectivity between all signals will be computed and a single | ||
| connectivity spectrum will be returned (this is not possible if a Granger | ||
| causality method is called). If "indices" is specified, seed and target | ||
| indices for each connection should be specified as nested array-likes. For | ||
| example, to compute the connectivity between signals (0, 1) -> (2, 3) and | ||
| (0, 1) -> (4, 5), indices should be specified as:: | ||
| For multivariate methods, this is handled differently. If ``indices`` is ``None``, | ||
| connectivity between all signals will be computed and a single connectivity spectrum | ||
| will be returned (this is not possible if a Granger causality method is called). If | ||
| ``indices`` is specified, seed and target indices for each connection should be | ||
| specified as nested array-likes. For example, to compute the connectivity between | ||
| signals (0, 1) -> (2, 3) and (0, 1) -> (4, 5), indices should be specified as:: |
There was a problem hiding this comment.
Line length and doubleticks
| where: :math:`\boldsymbol{D}(\Phi)` is the cross-spectral density | ||
| between seeds and targets transformed for a given phase angle | ||
| :math:`\Phi`; and :math:`\boldsymbol{a}` and :math:`\boldsymbol{b}` | ||
| are eigenvectors for the seeds and targets, such that | ||
| :math:`\boldsymbol{a}^T\boldsymbol{D}(\Phi)\boldsymbol{b}` | ||
| maximises coherency between the seeds and targets. Taking the | ||
| absolute value of the results gives maximised coherence. | ||
| where: :math:`\boldsymbol{D}(\Phi)` is the cross-spectral density between | ||
| seeds and targets transformed for a given phase angle :math:`\Phi`; and | ||
| :math:`\boldsymbol{a}` and :math:`\boldsymbol{b}` are eigenvectors for the | ||
| seeds and targets, such that :math:`\boldsymbol{a}^T\boldsymbol{D}(\Phi) | ||
| \boldsymbol{b}` maximises coherency between the seeds and targets. Taking | ||
| the absolute value of the results gives maximised coherence. |
There was a problem hiding this comment.
Only line length changes for eqs.
| ``method``. Each element is an array of shape (n_cons, [n_comps], n_freqs) or | ||
| (n_cons, [n_comps], n_fbands) if ``faverage`` is `True`. ``n_comps`` is present | ||
| for valid multivariate methods if ``n_components > 0``. | ||
| ``method``. Each element is an array of shape (n_cons, [n_comps,] n_freqs) or | ||
| (n_cons, [n_comps,] n_fbands) if ``faverage`` is ``True``. ``n_comps`` is | ||
| present for valid multivariate methods if ``n_components > 0``. |
There was a problem hiding this comment.
Optional dim syntax
| List of connectivity estimates between signals ``x`` and ``y`` | ||
| corresponding to the methods in ``method``. Each element is an array | ||
| with shape (n_freqs,) or (n_fbands) depending on ``faverage``. | ||
| List of connectivity estimates between signals ``x`` and ``y`` corresponding to | ||
| the methods in ``method``. Each element is an array with shape ``(n_freqs,)`` or | ||
| ``(n_fbands)`` depending on ``faverage``. |
There was a problem hiding this comment.
Line length, backticks
| seed and target patterns, respectively, each with shape ([n_comps], n_channels, | ||
| n_freqs) or ([n_comps], n_channels, n_fbands) depending on ``faverage``. | ||
| ``n_comps`` is present for valid multivariate methods if ``n_components > 0``. | ||
| ``n_channels`` is the largest number of channels across all connections, with | ||
| missing entries padded with ``np.nan``. | ||
| seed and target patterns, respectively, each with shape ``([n_comps,] | ||
| n_channels, n_freqs)`` or ``([n_comps], n_channels, n_fbands) depending on | ||
| ``faverage``. ``n_comps`` is present for valid multivariate methods if | ||
| ``n_components > 0``. ``n_channels`` is the largest number of channels across | ||
| all connections, with missing entries padded with ``np.nan``. |
| data : np.ndarray ([epochs], n_estimated_nodes, [components], [freqs], [times]) | ||
| The connectivity data that is a raveled array of ``(n_estimated_nodes, ...)`` shape. | ||
| The ``n_estimated_nodes`` is equal to ``n_nodes_in * n_nodes_out`` if one is | ||
| computing the full connectivity, or a subset of nodes equal to the length of | ||
| ``indices`` passed in. | ||
| data : array, shape ([epochs,] n_estimated_nodes[, components, freqs, times]) | ||
| The connectivity data that is a raveled array of ``(..., n_estimated_nodes, ...)`` | ||
| shape. The ``n_estimated_nodes`` is equal to ``n_nodes_in * n_nodes_out`` if one has | ||
| computed the full connectivity, or a subset of nodes equal to the length of the | ||
| arrays in ``indices`` passed in. |
There was a problem hiding this comment.
Optional dim syntax, possibility of epochs dim before nodes
| names : list | np.ndarray | None | ||
| The names of the nodes of the dataset used to compute | ||
| connectivity. If 'None' (default), then names will be | ||
| a list of integers from 0 to ``n_nodes``. If a list | ||
| names : array_like | None | ||
| The names of the nodes of the dataset used to compute connectivity. If ``None`` | ||
| (default), then names will be a list of integers from 0 to ``n_nodes``. If a list |
There was a problem hiding this comment.
Line length, backticks, simplify list or array to array-like
| indices : tuple of arrays | str | None | ||
| The indices of relevant connectivity data. If ``'all'`` (default), | ||
| then data is connectivity between all nodes. If ``'symmetric'``, | ||
| then data is symmetric connectivity between all nodes. If a tuple, | ||
| then the first list represents the "in nodes", and the second list | ||
| represents the "out nodes". See "Notes" for more information. | ||
| indices : tuple of array_like | ``'all'`` | ``'symmetric'`` | None | ||
| The indices of relevant connectivity data. If ``'all'`` (default), then data is | ||
| connectivity between all nodes. If ``'symmetric'``, then data is symmetric | ||
| connectivity between all nodes. If a tuple, then contains two array-likes where the | ||
| first array represents the "in nodes" (seeds), and the second array represents the | ||
| "out nodes" (targets). |
There was a problem hiding this comment.
Use literals, line length.
Also removed See "Notes" for more information since connectivity classes (only place this is used) have no notes section explaining indices.
Also suggest adding more context for in nodes as seeds and out nodes as targets, in line with terminology used elsewhere.
| mode : str (default 'multitaper') | ||
| The cross-spectral density computation method. Can be ``'multitaper'``, | ||
| ``'fourier'``, or ``'cwt_morlet'``. | ||
| mode : ``'multitaper'`` | ``'fourier'`` | ``'cwt_morlet'`` | ||
| The cross-spectral density computation method. |
| method : str, optional | ||
| The method name used to compute connectivity. | ||
| method : str | None | ||
| The method name used to compute connectivity (default ``None``). |
There was a problem hiding this comment.
Replace optional -> None
| f'"model" parameter must be one of ' f"(avg-epochs, dynamic), not {model}." | ||
| f'"model" parameter must be one of (avg-epochs, dynamic), not {model}.' |
| ``(model_order * (n_times - model_order), | ||
| n_signals * model_order)``. See Notes. | ||
| Y : np.ndarray | ||
| ``(model_order * (n_times - model_order), n_signals * model_order)``. See Notes. | ||
| Y : array | ||
| The predicted multivariate time-series. This will have shape | ||
| ``(model_order * (n_times - model_order), | ||
| n_signals * model_order)``. See Notes. | ||
| ``(model_order * (n_times - model_order), n_signals * model_order)``. See Notes. |
| con : array, shape (n_channels, n_channels) | Connectivity | ||
| The computed connectivity measure(s). | ||
| The connectivity data to plot. |
| Connectivity scores. Can be a square matrix, or a 1D array. If a 1D | ||
| array is provided, "indices" has to be used to define the connection | ||
| indices. | ||
| Connectivity scores. Can be a square matrix, or a 1D array. If a 1D array is | ||
| provided, ``indices`` has to be used to define the connection indices. |
There was a problem hiding this comment.
Below is all just line length, backticks and array-likes
mne_connectivity/base.py
Outdated
| conn : instance of Connectivity | ||
| conn : instance of Connectivity | SpectralConnectivity | TemporalConnectivity | SpectroTemporalConnectivity |
There was a problem hiding this comment.
for the return types, verbosity is fine if it's needed to be accurate. If there's a succinct way to sum it up, that can go in the textual description, so in addition to "The combined connectivity data structure" it could say something like "type will reflect the input data type (plain, spectral, temporal, spectrotemporal) but without the epoch dimension."
| sm_times=0, | ||
| sm_times=0.0, | ||
| sm_freqs=1, | ||
| sm_kernel="hanning", | ||
| padding=0, | ||
| padding=0.0, | ||
| mode="cwt_morlet", | ||
| mt_bandwidth=None, | ||
| n_cycles=7, | ||
| n_cycles=7.0, |
mne_connectivity/utils/docs.py
Outdated
| @@ -239,13 +229,13 @@ | |||
|
|
|||
| # Decoding attrs | |||
| docdict["filters_"] = """ | |||
| filters_ : tuple of array, shape=(n_signals, n_components) | |||
| filters_ : tuple of array, shape (2, n_signals, n_components) | |||
There was a problem hiding this comment.
shouldn't this be (2, (n_signals, n_components)) then? or maybe length-2 tuple of array, shape (n_signals, n_components)? Or even:
| filters_ : tuple of array, shape (2, n_signals, n_components) | |
| filters_ : tuple, shape (2,), of array, shape (n_signals, n_components) |
(which I don't love, but is in some sense the most consistent). I'm assuming that since this is seeds/targets, there's no guarantee of the two arrays actually having the same shape, so array_like, shape (2, n_signals, n_components) would be inaccurate right?
There was a problem hiding this comment.
I'm assuming that since this is seeds/targets, there's no guarantee of the two arrays actually having the same shape
You're completely right.
The variable dim size was being handled like this elsewhere:
So that should be updated depending on what is decided upon.
There are also these existing instances (not for multivariate connectivity) that use a similar pattern:
So they should also be updated.
Would you prefer something like tuple, shape (2,), of array, shape (n_signals, n_components) to length-2 tuple of array, shape (n_signals, n_components)?
There was a problem hiding this comment.
@larsoner which do you like better?
- length-2 tuple of array, shape (n_signals, n_components)
- tuple, shape (2,), of array, shape (n_signals, n_components)
or something else?
There was a problem hiding this comment.
Hmm in this case the two elements in the tuple can be different shapes, right? In that case the above all somewhat misleadingly make it seem like they have to be the same size... in which case you could just say it needs to be array-like of shape (2, n_signals, n_components). So here I would probably say
indices : tuple of length 2
The seed and target indices. The `ndarray`s inside the tuple have
shape `(n_seeds, n_components)` and `(n_targets, n_components)`, respectively.
... assuming n_components does match between the two. If it doesn't, then I'd name them differently like n_seed_components and n_target_components etc.
There was a problem hiding this comment.
+1, that is indeed better than anything I suggested.
There was a problem hiding this comment.
Okay, so for the decomposition filters and patterns there is this (n_components is the same for seeds and targets):
mne-connectivity/mne_connectivity/utils/docs.py
Lines 231 to 236 in fc4d5cd
mne-connectivity/mne_connectivity/utils/docs.py
Lines 238 to 243 in fc4d5cd
In check_indices this:
mne-connectivity/mne_connectivity/utils/utils.py
Lines 54 to 63 in fc4d5cd
In seed_target_indices this:
mne-connectivity/mne_connectivity/utils/utils.py
Lines 203 to 205 in fc4d5cd
And in seed_target_multivariate indices this (I don't see a clean way of getting around the variable dim size since here the number of channels can vary not only between seeds and targets, but also differ for each connection):
mne-connectivity/mne_connectivity/utils/utils.py
Lines 252 to 255 in fc4d5cd
There was a problem hiding this comment.
And in seed_target_multivariate indices this (I don't see a clean way of getting around the variable dim size since here the number of channels can vary not only between seeds and targets, but also differ for each connection):
If they vary I wouldn't put the shape in the top line at all, instead opting to describe them in the text more verbosely, like
".. can vary, resulting in seed and target shapes of (n_cons, n_channels_seed) and (n_cons, n_channels_target), respectively" or whatever makes sense.
There was a problem hiding this comment.
Here is the updated version:
mne-connectivity/mne_connectivity/utils/utils.py
Lines 246 to 264 in a9b5aeb
The notes section also has examples that provide further context.
I already did a pass through the whole diff yesterday. The "length-2 tuple of arrays" issue is the only thing left to sort out. |
Co-authored-by: Daniel McCloy <[email protected]>
Co-authored-by: Daniel McCloy <[email protected]>
|
Thanks both for the suggestions, and thanks @drammock for trawling through this mega diff! |
Addresses in particular #222 and #282 to fix all ambiguous single backtick entries, either giving them explicit intersphinx roles or making them double backticks like mentioned here.
Also generally brings the docstrings in line with what seem to be more modern MNE principles, including:
spec_conn_epochs/time()methodparam since that has loads of possibilities)optionalfrom docstring types and replace withNone, e.g.param : type, optional->param : type | None.arrayas a type instead ofnp.ndarrayorndarray.array_likeas a type where that is allowed in place ofarray.array of shape (...)orarray, shape=(...)witharray, shape (...).I realise it's a lot to go through so I'll try to highlight where actual changes to content have been made as opposed to line length shifts.