-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
MRG: Improve docstring of mne.io.Info and improve error messages in Info._attributes #9922
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
Conversation
For these I'm not sure, we'll have to look at the fiff-constants repo + mne/io/constants.py to figure it out. I'm guessing they are really only used for / by MEGIN / Elekta nowadays.
We use this one in the KIT reader, it identifies the MEG system in use
We use this to say whether active shielding (IAS) was active for the raw data
We set this when saving a forward solution (maybe inverse, too?) but in general people shouldn't set or use this themselves, and it shouldn't be a part of raw/epochs/evoked data. |
|
|
|
For Very similar to |
|
All good for me; the 4 last attributes that are missing from the docstring I also feel like the docstring is not very clear as it is, as it mixes attributes for Raw, Epochs, Evoked, Forward, ... + it mixes attributes that can be changed and read-only attributes. Anyway, this is already a small improvement compared to the previous docstring; feel free to change any description to suit you best. |
|
One way to find what these means is figure out the correspondence between you can see where it's actually read from file. (You can also in our code how we set it using other entries in the Can you try with the others, too? |
|
@larsoner That was already clear from your previous message, and I did look into:
For these 4 last ones, it was not very clear to me, and I could not make out a description I was confident enough with.
If you want I can post in a comment later today or tomorrow the relation/what I find in the fiff-constants and use cases in MNE for each of them. Maybe it will make sense to you and you can then propose type/description for each of them. |
Ahh sorry didn't realize I already told you how to do this :)
This behavior is hidden in the code somewhere, but just to give an example: Since we don't have a command_line exactly, we give the Python command. And the working_dir is the current directory that was used (which isn't really very useful, but we add it anyway).
Xplotter is a Neuromag plotting utility. We never change/set this ourselves, we just preserve it if it already exists in the object.
For this I see: So I don't think it should exist anymore. If it does, it's probably a bug. |
|
@larsoner Ok, it does start to make some sense. So Docstring proposition: For the Xplotter, what is the python type associated with it? Description could be: For
Let's remove it for good in this PR. |
Does this end up in
If you follow this to So it's fine to make this an error nowadays, just like any other invalid key.
No this is pickling or something I think,
This is also not the same as our |
… this is now covered by locking/unlocking.
|
@larsoner All 4 correct ;) I removed the unnecessary warning in Any idea on the type of the variable in |
|
Ok, well that settles it. Now all attributes from |
|
Can you add an entry in |
|
Done, let's see if I added the entry correctly or if the doc build is unhappy 😅 |
larsoner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments, otherwise LGTM
|
Thanks @mscheltienne ! |
* upstream/main: remove dead link (no suitable alternative) [skip actions][skip azp] (mne-tools#9979) MRG: Improve docstring of mne.io.Info and improve error messages in Info._attributes (mne-tools#9922) MRG: Add "interaction" param to CoregistrationUI (mne-tools#9972) MRG, FIX: Fix interior check (mne-tools#9968)
Next small PR to improve Info.
Most of the long list of attributes in the docstring is sorted in alphabetical order; except attributes that were added later on and where
.. versionadded:: x.xxis specified. It complicates navigation on the API Page, and it makes it difficult to find the docstring of the attribute we are looking for.Some error messages in
_attributescould be improved. I added the methods to use fornchanandprojs, but I don't know which methods/functions should be proposed to the user for the remaining attributes.Some attributes are not present in the docstring. Missing attributes are:
command_linefilenamekit_system_idmaxshieldmeas_filemri_filemri_head_tmri_idworking_dirxplotter_layoutAre they all deprecated and present for backward compatibility only?
If this is the case, I will add it to the error message.
For now, in the docstring, it says
bads,description,experimenter, andline_freq.But, in practice it also includes
dev_head_t,subject_info,device_infoandhelium_info. Do you agree with this extended list or would you prefer to have some of those become locked?