Skip to content
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

Open
csabella opened this issue Mar 28, 2019 · 17 comments
Open

timeit: Additional changes for autorange #80642

csabella opened this issue Mar 28, 2019 · 17 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@csabella
Copy link
Contributor

BPO 36461
Nosy @stevendaprano, @alessandrocucci, @csabella, @mangrisano
PRs
  • bpo-36461: Added the total_time parameter to the timeit.autorange() function. #12953
  • bpo-36461: timeit - Additional changes for autorange #12954
  • 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:

    assignee = None
    closed_at = None
    created_at = <Date 2019-03-28.09:49:28.109>
    labels = ['3.7', '3.8', 'type-feature', 'library', 'easy']
    title = 'timeit: Additional changes for autorange'
    updated_at = <Date 2019-06-01.10:54:11.435>
    user = 'https://github.com/csabella'

    bugs.python.org fields:

    activity = <Date 2019-06-01.10:54:11.435>
    actor = 'steven.daprano'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2019-03-28.09:49:28.109>
    creator = 'cheryl.sabella'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 36461
    keywords = ['patch', 'easy']
    message_count = 12.0
    messages = ['339025', '339027', '340541', '343388', '343394', '343400', '343403', '343409', '343454', '343484', '344166', '344167']
    nosy_count = 4.0
    nosy_names = ['steven.daprano', 'acucci', 'cheryl.sabella', 'mangrisano']
    pr_nums = ['12953', '12954']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue36461'
    versions = ['Python 3.7', 'Python 3.8']

    @csabella
    Copy link
    Contributor Author

    bpo-6422 implemented the autorange function for timeit, but in msg272704, Steven D'Aprano outlined follow-up change requests to that patch.

    • make the 0.2s time configurable;
    • have timeit and repeat methods (and functions) fall back
      on autorange if the number is set to 0 or None.

    Opening this ticket for those change requests.

    @csabella csabella added 3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir easy type-feature A feature request or enhancement labels Mar 28, 2019
    @csabella
    Copy link
    Contributor Author

    Assigning to @Mariatta for the sprints.

    @alessandrocucci
    Copy link
    Mannequin

    alessandrocucci mannequin commented Apr 19, 2019

    Hello @Mariatta,
    if this is simple I would like to work on that, can I?
    Thanks!

    @csabella
    Copy link
    Contributor Author

    @steven.daprano, would you be able to review the pull requests based on your original concept for this change? Thank you!

    @stevendaprano
    Copy link
    Member

    @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
    browser that works properly with github. But I'll do what I can.

    @stevendaprano
    Copy link
    Member

    (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 max_time_taken seconds, not at most.

    So I think a better name might be something like target_time. It suggests a time we are targeting, not one we promise to hit precisely, it doesn't mislead into thinking it will be the total time. And it is a bit shorter without being cryptically short.

    Anyone have any better suggestions for the parameter name?

    Likewise I suggest default_target_time for the global (line 62).

    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.)

    @stevendaprano
    Copy link
    Member

    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 Timer.timeit() or Timer.repeat() to specify a custom target time, is that right?

    @stevendaprano
    Copy link
    Member

    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 autorange method, although I prefer the name "target_time". (But I'm open to alternatives.)

    So I think a good approach might be a hybrid between mangrisano's design, and the design by Alessandro:

    1. the constructor takes the target time and records it as an attribute;
    def __init__(self, ..., target_time=default_target_time):
        self.target_time = target_time
    1. when the timeit method is used, if number=0 the target time is taken from self.target_time and passed to autorange;
    • if the autorange method is called directly, the caller can override the target time by passing it as argument; otherwise the default value is looked up from the instance attribute.

    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.)

    @alessandrocucci
    Copy link
    Mannequin

    alessandrocucci mannequin commented May 25, 2019

    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.

    @mangrisano
    Copy link
    Mannequin

    mangrisano mannequin commented May 25, 2019

    I agree with *target_time*. I'm working on it and soon I'm going to update the pr.

    @stevendaprano
    Copy link
    Member

    Sorry for the late reply.

    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?

    Fair question. On rethinking, I'm okay with an explicit check for None
    or zero, if number is None or number < 0 but I don't like the idea
    of accepting any falsey value.

    Calling the function with False is fine, since False == 0 but I don't
    think it is fine to call the function with (say) [] or {} or "", which
    are all falsey values.

    @stevendaprano
    Copy link
    Member

    if number is None or number < 0

    Sorry, that should be number == 0

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @CohenAriel
    Copy link

    CohenAriel commented Aug 13, 2023

    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?

    @gabrielfougeron
    Copy link

    Hi @CohenAriel

    Did you end up opening a new PR? I'd love to see this 0.2 be configurable.

    Cheers

    @CohenAriel
    Copy link

    CohenAriel commented Dec 30, 2023 via email

    @gabrielfougeron
    Copy link

    True.... I hope someone on the inside can share thoughts as to why.....

    @encukou
    Copy link
    Member

    encukou commented Mar 19, 2024

    @stevendaprano is not active on GitHub nowadays. Someone else needs to take over, learn the context, study the previous discussions, determine what changed since the last PR.
    I can now dedicate some time for that.

    Do you want to open a new PR, merging in the main branch?

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants