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.

Sign up for GitHub

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

Jump to bottom

Changed per-pipeline timings to store as a float #4160

Merged
merged 4 commits into from
Apr 27, 2023
Merged

Changed per-pipeline timings to store as a float #4160

merged 4 commits into from
Apr 27, 2023

Conversation

christopherbunn
Copy link
Contributor

@christopherbunn christopherbunn commented Apr 26, 2023 •
edited
Loading

Resolves #4162

@codecov
Copy link

codecov bot commented Apr 26, 2023 •
edited
Loading

Codecov Report

Merging #4160 (6d4c169) into main (7b2b5d8) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #4160     +/-   ##
=======================================
- Coverage   99.7%   99.7%   -0.0%     
=======================================
  Files        349     349             
  Lines      37781   37777      -4     
=======================================
- Hits       37664   37660      -4     
  Misses       117     117
Impacted Files Coverage Δ
evalml/utils/logger.py 100.0% <ø> (ø)
evalml/automl/automl_search.py 99.6% <100.0%> (-<0.1%) ⬇️
evalml/tests/automl_tests/test_automl.py 99.6% <100.0%> (ø)
evalml/tests/utils_tests/test_logger.py 100.0% <100.0%> (ø)

@christopherbunn christopherbunn changed the title Store pipeline timings as a float Store per-pipeline timings as a float Apr 27, 2023
@christopherbunn christopherbunn changed the title Store per-pipeline timings as a float Changed per-pipeline timings to store as a float vs. a string value Apr 27, 2023
@christopherbunn christopherbunn changed the title Changed per-pipeline timings to store as a float vs. a string value Changed per-pipeline timings to store as a float Apr 27, 2023
@christopherbunn christopherbunn marked this pull request as ready for review April 27, 2023 19:22
jeremyliweishih
jeremyliweishih approved these changes Apr 27, 2023
View reviewed changes
Copy link
Contributor

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

eccabay
eccabay reviewed Apr 27, 2023
View reviewed changes
Copy link
Contributor

@eccabay eccabay left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! One request before approval - could you replace elapsed_time = time_elapsed(self.progress.start_time) (line 1170) as well? It'd keep things more consistent.

@christopherbunn christopherbunn enabled auto-merge (squash) April 27, 2023 20:06
@christopherbunn christopherbunn merged commit b331a7d into main Apr 27, 2023
28 checks passed
@christopherbunn christopherbunn deleted the store_timings branch April 27, 2023 20:28
@chukarsten chukarsten mentioned this pull request May 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremyliweishih jeremyliweishih jeremyliweishih approved these changes

@eccabay eccabay eccabay approved these changes

@chukarsten chukarsten Awaiting requested review from chukarsten

@fjlanasa fjlanasa Awaiting requested review from fjlanasa

Assignees

@christopherbunn christopherbunn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Store per-pipeline timings as a numerical type
3 participants
@christopherbunn @jeremyliweishih @eccabay