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

Add units to timings in node labels #58

Open
robinst opened this issue Feb 22, 2019 · 4 comments
Open

Add units to timings in node labels #58

robinst opened this issue Feb 22, 2019 · 4 comments

Comments

@robinst
Copy link
Contributor

robinst commented Feb 22, 2019

Currently, the timings on node labels are shown like this: 0.1234

It's not immediately clear that that's in seconds. Can we add a " s" to the label? Or maybe even consider changing it to milliseconds and adding " ms".

I'm not familiar with all the input formats and if they have the timing information in seconds or not, but if it's just a matter of changing the label formatting, I'm happy to raise a PR.

@jrfonseca
Copy link
Owner

I agree having a time unit (ms in particular) would be better.

But I also don't know off hand if time units are being consistently parsed for all formats. This is because until recently we didn't care about exact time units, as we always computed the relative time (against total.)

I'm afraid I don't have much time to look into this myself, but I'd accept patches.

CC'ing @robinst

@robinst
Copy link
Contributor Author

robinst commented Feb 25, 2019

But I also don't know off hand if time units are being consistently parsed for all formats

Would it be acceptable to make this dependent on the format? I'm currently using pstats and I think it always shows times in seconds. So if you use pstats, you'd get units in labels but if you use another format you would get what you see now (just a number).

@jrfonseca
Copy link
Owner

I think it might be easier to apply a (format-specific) conversion factor when parsing the stats, than to carry around the unit. I'm OK with storing internally all times in seconds, and then perhaps show in ms for times less than 1 second.

That is, my suggestion is to presume for now all formats represent time in seconds, and then fixup later the parsing of formats that don't.

@robinst
Copy link
Contributor Author

robinst commented Feb 26, 2019

My idea was to just add the "ms" if we're certain about what unit a time is in. But if you're ok with potentially showing the wrong unit for some formats, I can raise a PR for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants