-
Notifications
You must be signed in to change notification settings - Fork 163
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
Move towards making set_probe in place by default #2798
base: main
Are you sure you want to change the base?
Move towards making set_probe in place by default #2798
Conversation
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.
For me I liked the always return (i.e. in_place always False), but from the debate it looks like it went the other direction. (That's what I get for being silent).
So my only comment would more be discoverability/understandability. We distinguish between set_probe
and set_probegroup
at the user-level, but now this function does both. Why? shouldn't we have a create_with_probe
and a separate create_with_probegroup
to maintain the previous api for users. We don't really have the concept of probes
right? Instead you choose between A probe and A probegroup neither being plural for the api.
EDIT: just to be clear I do support making this a one option function in general. The rest of the api always returns a new object so having this function be the one function that gives users the choice is super confusing. For me I was leaning toward return just for api consistency but otherwise I don't care which way it works.
@@ -225,6 +234,27 @@ def set_probes(self, probe_or_probegroup, group_mode="by_probe", in_place=False) | |||
|
|||
return sub_recording | |||
|
|||
def create_with_probes(self, probe_or_probegroup, group_mode="by_probe"): |
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.
I wonder if it should be create_with_probe
vs probes
.
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.
Ther are two functions, the one with the plural name is the most general as it takes both probe a list or a probe group, so that's where I want the the warnin to be. I wish there was only one function and if you see the diff in the changes it seems that that was the intention of one point.
Simplifying probe setting could be a issue of its own, I am not making any changes in that direction in this PR. It is just to get the ball rolling to make the behaivor of the function consistent with setter semantics.
Personally I don't mind if it returns a recording (for API consistency) or sets in place. But if it returns a recording I think it should be renamed from For |
Joe's comment actually reminded me the point of set so I've changed my mind. set should be in_place so I retract my previous statement.
Yep exactly! |
As mentioned above, there are two functions Am I missing something? |
I was referring to the new function you made called
to mimic the functions I normally use |
See line 99 which internally uses |
All the functions are public so I think you would have to ask @samuelgarcia and @alejoe91 what is the "right way" (intendend) that they envisioned or want. Usually they prefer general functions that handle a bunch of cases (see that they prefer to use |
Agreed. I don't think I've ever seen us document |
Looking the code base it seems that |
I just searched the docs and set_probe is mentioned like 5 times. set_probegroup is mentioned once and set_probes is never mentioned (again based on the search function). |
Which search function are you using? I searched before the claim above. The one use case is here and by someone that is not "us": |
The search function on readthedocs. I'm talking about rtd documentation. |
Got you. I think the evidence points in the direction of your claim, though. I will make these two methods. I think we probably should make the most general method |
Another issue here: #2895 |
Should fix #2193