Skip to content

Conversation

@mscheltienne
Copy link
Member

I don't see why this method should be missing from Info (plus I actually encountered a case where I would have liked to have it). Docstrings are almost identical.. but not quite, which is why I didn't copy it with the decorator.

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx @mscheltienne

I would just see if we can avoid the big copy paste of the docstring

@mscheltienne
Copy link
Member Author

How about a docdict entry grouping all parameters from pick_types at once?
Let's see if it renders correctly.

@larsoner
Copy link
Member

larsoner commented Nov 24, 2021

I am -0.5 on this. I think for new and even intermediate users it will be too easy for them to accidentally do raw.info.pick_types(...) and thus end up with a silently broken raw instance when they meant to do raw.pick_types. I also worry about people seeing raw.pick_types and info.pick_types and not knowing which one to use anymore.

Currently to get the functionality in this PR, you can do pick_info(info, pick_types(info, ...), copy=False) which is more cumbersome, but I see that as in some sense a feature because it's much harder to do accidentally for the common use cases people have (where they should almost always do raw.pick_types). And I say this as someone with many advanced use cases (simulations, messing with internal code, etc.) where I end up pick_infoing often.

I think if we really want to add this, we need to first add some way for an info to know if it has a parent instance. That way if you do raw.info.pick_types, we can raise an error, or maybe better, just call parent.pick_types under the hood. I don't think this will be trivial, though, and could have some nasty side effects (e.g., if you have some parallel function that you pass raw.info to, when that raw.info gets pickled it will pickle the raw object, too, which could take a ton of memory). It's possible that using a WeakRef could get us past this pickling problem, not sure...

There are some other potential advantages to tracking the parent, for example each info should only be the child of a single instance (we will have bugs if it's not the case; and we can clear the parent when it's .copy()ed).

@mscheltienne
Copy link
Member Author

That's a very good point.. However, tracking the parent will require a lot more work..
Following your logic, Info.pick_channels should be deprecated as it can lead to the same broken raw instance.

@agramfort
Copy link
Member

agramfort commented Nov 24, 2021 via email

@mscheltienne
Copy link
Member Author

mscheltienne commented Nov 24, 2021

No problem 😉 I'll keep this one in mind for when Info has knowledge of its parent.
FYI for .pick_channels, it was added in #7029

@larsoner
Copy link
Member

I think I'm +1 for deprecating info.pick_channels and tell people that they should use inst.pick_channels instead (experts will know to use pick_info if they need to I think)

@agramfort
Copy link
Member

agramfort commented Nov 25, 2021 via email

@mscheltienne mscheltienne deleted the add_pick_types_to_info branch November 25, 2021 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants