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

feature request: easier control on renderer #4

Closed
gilbh opened this issue Nov 11, 2021 · 5 comments
Closed

feature request: easier control on renderer #4

gilbh opened this issue Nov 11, 2021 · 5 comments

Comments

@gilbh
Copy link

gilbh commented Nov 11, 2021

Hi,

I noticed there is no carriage return in the ticktock message, so I wrote the following, but I was wondering if there could be a less verbose way of doing this:

from ticktock import ticktock
from ticktock.timer import ClockCollection, set_collection
from ticktock import renderers

set_collection(
    collection=ClockCollection(renderer=renderers.StandardRenderer(format=renderers.StandardRenderer()._format + '\n')))


@ticktock
def test():
    for aa in range(100_000):
        pass


test()
print('done')

Thanks!

@victorbenichoux
Copy link
Owner

Hi @gilbh,

Thanks for the issue! At the moment there isn't anything much simpler than this.

However, I think you just pointed to a bug here, I should probably add a carriage return at the end of the block of messages from ticktock. Note that your solution breaks when we display multiple clocks 😬 .

I will fix this ASAP!

@gilbh
Copy link
Author

gilbh commented Nov 12, 2021

Nice. So: adding a carriage return is a good step, and please let me know when it is up.

Also, regarding: At the moment there isn't anything much simpler than this. -- what should be cut out (at least from my perspective as the user of this library) is the need to import additional objects and then make this complicated to them. Ideally, I would just add the formatting string to either the tick call or the ticktock object. Perhaps also having an additional flag make_default that makes the last formatting string the default for future calls ...

This is a superficial suggestion. Not sure if possible/fits your design.

Best!

@victorbenichoux
Copy link
Owner

Hi @gilbh

Thanks for the feedback, it's much appreciated!
And obviously at this stage I am very happy to make changes to the library to suit the needs of anyone who wants to use it 😄 , so do feel free!

First, here is what exists now to globally change the formatting of rendered clocks.

There is an "f-string" like way to change the format of all clocks, for example:

StandardRenderer(format="{mean} max={max} count={count}")

As you correctly point out, it is a pain to set this globally (need to import set_collection, ClockCollection, StandardRenderer, etc).

It is also possible to set this format string using an env var:

TICKTOCK_DEFAULT_FORMAT="{mean} max={max} count={count}"

Which is, well, useful I guess, but also pretty cumbersome. Not sure why I put that in 🤔 .

Now, as for the next steps.

I do like your suggestion of setting the format through tick or ticktock calls, because there is no need to import anything else. The only issue with this is that the renderer is global to all clocks, and letting each tick have a different format requires supporting a renderer per clock. It takes a bit more work given the current implementation, but can be done.

TBH, I initially did not see a use case for this but I am starting to see why one would want to do it. For example just counting occurrences of a tick, while showing timing for another, or track the max timing of some clocks but not others.

I am not a big fan of having a make_default kwarg, because it makes the renderer depend on the evaluation order of the ticks. For example, I like using ticks deep inside functions in libraries that I call, and I am not always sure which is found first. As a result, I would rather implement a global set_format to set the format string.

Anyhow, this is all interesting: I will start with the easy fixes:

  • push a fix with the carriage return thing,
  • add a set_format to save us the additional imports
  • finally see how long it would take to support different renderers for each clock.

@gilbh
Copy link
Author

gilbh commented Nov 12, 2021

Yes, I very much agree with what you write here. Do keep me posted.

This was referenced Nov 13, 2021
@victorbenichoux
Copy link
Owner

victorbenichoux commented Nov 14, 2021

I have made a release with set_format
Thanks for contributing!

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

No branches or pull requests

2 participants