DOC: document onset, duration, description, and ch_names attributes of mne.Annotations#13680
DOC: document onset, duration, description, and ch_names attributes of mne.Annotations#13680Famous077 wants to merge 32 commits intomne-tools:mainfrom
Conversation
for more information, see https://pre-commit.ci
|
@Famous077 can you please sign in to CircleCI via the sign in with Github option? After you do this and push a commit to this branch, the build docs job should run. |
…github.com/Famous077/mne-python into doc/annotations-onset-duration-description
…github.com/Famous077/mne-python into doc/annotations-onset-duration-description
for more information, see https://pre-commit.ci
drammock
left a comment
There was a problem hiding this comment.
on main users are free to mutate my_annots.duration pretty freely.. At least compared to the set_annotations route that contains some checks [...] I'm not sure if that was just an oversight. Do we want to be more strict about correctness in these setters, similar to what's done in _check_o_d_s_c_e?
After a real-time chat with @scott-huberty, I think it makes sense to leverage our code in _check_o_d_s_c_e to validate the input to the setters. May require a slight refactor of _check_o_d_s_c_e to avoid duplicating code (so that, e.g., checking descriptions can be done independently of checking onsets, durations, etc)
| .. _Ezequiel Mikulan: https://github.com/ezemikulan | ||
| .. _Fahimeh Mamashli: https://github.com/fmamashli | ||
| .. _Famous Raj Bhat: https://github.com/Famous077 | ||
| .. _Famous077: https://github.com/Famous077 |
There was a problem hiding this comment.
this will break the xref in previously-merged changelog entries, please use the same name for all your PRs.
|
also, please add |
@drammock , Sorry for the delay, due to exams . Mistakely I mentioned issue number inside the default description template . Now, I have updated accordingly. |
Hi @drammock, I have addressed all the feedbacks. Can you review it whenever you feel free. Let me know if anything I missed to implement. Waiting for you feedback. |
@Famous077 you missed a comment here and probably here Here is a small snippet to help give you an idea of the hard edges we are trying to help users avoid. This is why Dan suggested looking into what checks we are doing in fpath = mne.datasets.sample.data_path() / "MEG" / "sample" / "sample_audvis_raw.fif"
raw = mne.io.read_raw_fif(fpath)
annots = mne.Annotations(onset=[1,3,2,4], duration=0, description="foo")
raw.set_annotations(annots)
raw.annotations.description = raw.annotations.description[:2] # 🚨 Now this has a different length than onset, duration etc |
|
Hi @scott-huberty , Changes have been implemented as per the suggestion. Can you verify it is working correctly or not? |
…etter validation error
|
@scott-huberty LMK when you're satisfied and I'll take another look |
…private attribute usage in append
|
Hi @scott-huberty , I have addressed your suggestion, All 170 tests pass. Please review when you get a chance if anything else need to be implemented. |
mne/annotations.py
Outdated
| Annotations.duration : Duration of each annotation. | ||
| Annotations.description : Description of each annotation. |
There was a problem hiding this comment.
| Annotations.duration : Duration of each annotation. | |
| Annotations.description : Description of each annotation. | |
| :attr:`~mne.Annotations.duration` | |
| :attr:`~mne.Annotations.description` |
I think this should work..
|
Hi @scott-huberty , Updated all See Also sections to use :attr: cross-references as suggested. All tests still pass. Let me know if anything else needs changing. |
scott-huberty
left a comment
There was a problem hiding this comment.
@drammock ready for you to take a final review.
|
Hi @drammock , I have addressed all the suggestions given. Can you review it whenever you get time. Waiting for your feedback. |
drammock
left a comment
There was a problem hiding this comment.
jointly reviewed with @scott-huberty
mne/annotations.py
Outdated
|
|
||
|
|
||
| def _check_o_d_s_c_e(onset, duration, description, ch_names, extras): | ||
| def _check_onset(onset): |
There was a problem hiding this comment.
this should take an n parameter too, like _check_duration does
mne/annotations.py
Outdated
|
|
||
| @onset.setter | ||
| def onset(self, onset): | ||
| onset = _check_onset(onset) |
There was a problem hiding this comment.
| onset = _check_onset(onset) | |
| onset = _check_onset(onset, n=len(self._onset)) |
mne/annotations.py
Outdated
| if len(onset) != len(self._duration): | ||
| raise ValueError( | ||
| f"Length of onset ({len(onset)}) must match the length of " | ||
| f"existing duration ({len(self._duration)})." | ||
| ) |
There was a problem hiding this comment.
with other suggested changes, I think this could then go away.
mne/annotations.py
Outdated
|
|
||
| @duration.setter | ||
| def duration(self, duration): | ||
| n = len(self._onset) |
There was a problem hiding this comment.
| n = len(self._onset) | |
| n = len(self._duration) |
this shouldn't make a difference... but as written it looks like a typo to be checking onset length in the duration setter. More maintainable this way.
mne/annotations.py
Outdated
| if len(duration) != n: | ||
| raise ValueError( | ||
| f"Length of duration ({len(duration)}) must match the length of " | ||
| f"existing onset ({n})." | ||
| ) |
There was a problem hiding this comment.
shouldn't this be guaranteed by _check_duration succeeding?
mne/annotations.py
Outdated
| if len(description) != n: | ||
| raise ValueError( | ||
| f"Length of description ({len(description)}) must match the " | ||
| f"length of existing onset ({n})." | ||
| ) |
There was a problem hiding this comment.
should be guaranteed by success of _check_description
mne/annotations.py
Outdated
|
|
||
| @ch_names.setter | ||
| def ch_names(self, ch_names): | ||
| n = len(self._onset) |
There was a problem hiding this comment.
| n = len(self._onset) | |
| n = len(self._ch_names) |
mne/annotations.py
Outdated
| if len(ch_names) != n: | ||
| raise ValueError( | ||
| f"Length of ch_names ({len(ch_names)}) must match the length of " | ||
| f"existing onset ({n})." | ||
| ) | ||
| self._ch_names = ch_names |
There was a problem hiding this comment.
should be guaranteed by successful _check_ch_names right?
mne/annotations.py
Outdated
| self._onset = state["onset"] | ||
| self._duration = state["duration"] | ||
| self._description = state["description"] | ||
| self._ch_names = state["ch_names"] | ||
| self._extras = state.get("_extras", [None] * len(self._onset)) |
There was a problem hiding this comment.
use the new _check_* functions (or check_odsce) here before setting the private attributes.
mne/annotations.py
Outdated
| # Write directly to private attributes to avoid triggering the public | ||
| # setter validation, which would raise an error due to temporary length | ||
| # mismatches while fields are being extended one at a time. | ||
| # The data is already validated by _check_o_d_s_c_e above. |
There was a problem hiding this comment.
this comment is in the wrong place??
…ove comment to correct location
for more information, see https://pre-commit.ci
…in HEDAnnotations.__setstate__
|
Hi @drammock , Hi, I've implemented the required changes as per the suggestions. Could you please review the PR when you get a chance and let me know if anything needs to be updated or improved. Thank you |
| def _check_onset(onset, n=None): | ||
| """Convert and validate onset to a 1D float array.""" | ||
| onset = np.atleast_1d(np.array(onset, dtype=float)) | ||
| if onset.ndim != 1: | ||
| raise ValueError( | ||
| f"Onset must be a one dimensional array, got {onset.ndim} (shape " | ||
| f"{onset.shape})." | ||
| ) | ||
| if n is not None and len(onset) != n: |
There was a problem hiding this comment.
I don't see any instances in which we pass _check_onset(..., n=None). Do we really need to make it optional?
There was a problem hiding this comment.
Hi @scott-huberty , The n=None default is needed because _check_onset is called in two places - once inside _check_o_d_s_c_e without n, since length validation is handled there separately, and once in the onset setter with n=len(self._onset) to validate against existing duration. So making it optional lets us reuse the same function in both contexts without duplicating code. Happy to refactor if you prefer a different approach.
fixes #12379
What does this implement/fix?
--> I have Converted
onset,duration,description, andch_namesfrom plaininstance attributes to documented properties in
mne/annotations.py,so they appear in the API reference with proper NumPy-style docstrings.
Additional information
--> Even experienced MNE users were unaware these attributes existed because
they did not appear in the generated API documentation. A changelog entry
has been added in
doc/changes/dev/12379.other.rst.