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

Shortcut adding zero timedelta in parse_time() #5108

Merged
merged 2 commits into from
Mar 19, 2021

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Mar 17, 2021

xref #5107 - there is a non-trival performance overhead in adding a zero timedelta, when instead one can just check for zero and not add the timedelta. This should possibly go upstream to astropy too?

@dstansby dstansby requested a review from a team as a code owner March 17, 2021 20:49
@dstansby dstansby added the time Affects the time submodule label Mar 17, 2021
@ayshih
Copy link
Member

ayshih commented Mar 18, 2021

I recommend a complete change in the fix approach. time_delta can only have two values: TimeDelta(0*u.day) and TimeDelta(1*u.day). I suggest getting rid of time_delta entirely by changing the private function _regex_parse_time() to return a Boolean instead for its second argument, and that Boolean would drive if a day needs to be added in convert_time_str().

@dstansby
Copy link
Member Author

Good idea, thanks for digging a bit deeper; modified as such.

@Cadair
Copy link
Member

Cadair commented Mar 19, 2021

I think parse_time might be a great place to start writing some benchmarks?

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

Successfully merging this pull request may close these issues.

4 participants