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
timeit: Additional changes for autorange #80642
Comments
bpo-6422 implemented the autorange function for timeit, but in msg272704, Steven D'Aprano outlined follow-up change requests to that patch.
Opening this ticket for those change requests. |
Assigning to @Mariatta for the sprints. |
Hello @Mariatta, |
@steven.daprano, would you be able to review the pull requests based on your original concept for this change? Thank you! |
With difficulty... for technology/financial reasons, I don't have a |
(Sorry, I can't comment on Github.) Looking at PR 12954: I'm not sure about the API for making the time used by autorange configurable. The time taken is only used when autoranging, but the API takes it as an argument to the constructor even if it won't be used. I haven't thought deeply about this for a few years, so now I'm wondering if this is the best API. Let me think some more. However, the parameter name "max_time_taken" is certainly wrong: it can be wrongly read as meaning the maximum time the sample code will run, which is not the case. Also, it is not a maximum time, it is a minimum time: the autoranger increases the number of loops until it takes at least So I think a better name might be something like Anyone have any better suggestions for the parameter name? Likewise I suggest With the longer parameter list, there's a couple of places that the line length seems to exceed 79 columns, e.g. lines 244, 272. Line 176: I think that should be ``if number == 0:`` since we don't want to accept any arbitrary falsey value, just zero. NEWS line 4: take out the reference to None. (I haven't reviewed the tests at this stage.) |
Looking at PR 12953: The only API for setting the target time is by passing it to the autorange method directly. So I think that there's no way for the caller of |
Michele, Alessandro, thank you both for your work! And thank you Cheryl for managing this ticket. I like mangrisano's design where the target time is passed as an argument to the So I think a good approach might be a hybrid between mangrisano's design, and the design by Alessandro:
def __init__(self, ..., target_time=default_target_time):
self.target_time = target_time
So something like def autorange(self, callback=None, target_time=None):
if target_time is None:
target_time = self.target_time
... Please suggest alternatives or point out anything I may have missed, and thank you all again! (I'm now going to be away from the keyboard for at least the next 18 hours so if I don't reply quickly, that's why.) |
Thanks Steven, I like the new name "target_time". Just a question: why we need to check ``if number == 0:``? In the proposal you asked for None too. What changed? Even if the function is called with False, will it hurts to keep the default value? I'll try to implement your ideas during this weekend. |
I agree with *target_time*. I'm working on it and soon I'm going to update the pr. |
Sorry for the late reply.
Fair question. On rethinking, I'm okay with an explicit check for None Calling the function with False is fine, since False == 0 but I don't |
Sorry, that should be number == 0 |
It looks like both the PRs have been inactive for four years, so is this still relevant? If so, can I open a new PR? |
Hi @CohenAriel Did you end up opening a new PR? I'd love to see this 0.2 be configurable. Cheers |
Haven't opened a PR yet, but I can try to open one.
It looks like the other PRs didn't get reviewed though.
…On Fri, 29 Dec 2023 at 23:48, Gabriel Fougeron ***@***.***> wrote:
Hi @CohenAriel <https://github.com/CohenAriel>
Did you end up opening a new PR? I'd love to see this 0.2 be configurable.
Cheers
—
Reply to this email directly, view it on GitHub
<#80642 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AR3VT2RULT7JCTHDMAPQV2DYL43EFAVCNFSM6AAAAAA3O4VTIWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZSGM2TGNRZGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
True.... I hope someone on the inside can share thoughts as to why..... |
Do you want to open a new PR, merging in the main branch? |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: