Skip to content

store & report dataop evaluation timing - #1596

Merged
rcap107 merged 6 commits into
skrub-data:mainfrom
jeromedockes:dataop-duration
Sep 12, 2025
Merged

store & report dataop evaluation timing#1596
rcap107 merged 6 commits into
skrub-data:mainfrom
jeromedockes:dataop-duration

Conversation

@jeromedockes

Copy link
Copy Markdown
Member

This adds to the dataop's report the time it took to evaluate each node

ATM it is shown here in the full_report:

image

zoom:

image

I also took the opportunity to do a bit of refactoring & slightly improving comments in the run() function in which the timing is added

+ small refactoring of eval function
@rcap107 rcap107 added this to the 0.6.2 milestone Sep 11, 2025
{% if eval_duration is not none %}
<div>
<h3>Computation time</h3>
{{ "{:.2g}".format(eval_duration) }} s

@rcap107 rcap107 Sep 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think minutes should also be added, I could imagine some nodes taking longer than 60 seconds to fit on larger data

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok I improved a bit the formatting:

image

@rcap107

rcap107 commented Sep 11, 2025

Copy link
Copy Markdown
Member

This looks really good, thanks a lot @jeromedockes

Maybe this should also be added to the param searches? though in that case it would just be the sum of all runs.

@jeromedockes

Copy link
Copy Markdown
Member Author

Maybe this should also be added to the param searches? though in that case it would just be the sum of all runs.

Maybe, yes. ATM we don't have those kind of detailed node-by-node reporting for the param searches, I guess what is interesting in param search is more aggregated information such as the mean fit time of the whole pipeline as shown in the results_ or parallel coord plot, otherwise it would be a bit unwieldly. and then you might want to dive in and do a full report of the best_learner_

in any case I would suggest exploring that in another issue; now at least the information is available in the node

@jeromedockes jeromedockes added the data_ops Something related to the skrub DataOps label Sep 11, 2025
@rcap107

rcap107 commented Sep 12, 2025

Copy link
Copy Markdown
Member

in any case I would suggest exploring that in another issue; now at least the information is available in the node

Yes better do that

rcap107
rcap107 previously approved these changes Sep 12, 2025

@rcap107 rcap107 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me! Thanks a lot @jeromedockes

@rcap107
rcap107 dismissed their stale review September 12, 2025 08:16

I forgot one change

@rcap107

rcap107 commented Sep 12, 2025

Copy link
Copy Markdown
Member

Can you add a mention of the evaluation time in the docstring for the full_report? Aside from that, the PR is good.

We should have a proper section on the functions of the full report in the user guide as well (in a separate PR).

@rcap107
rcap107 merged commit 822999a into skrub-data:main Sep 12, 2025
26 checks passed
@jeromedockes
jeromedockes deleted the dataop-duration branch September 12, 2025 18:00
rcap107 pushed a commit to rcap107/skrub that referenced this pull request Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

data_ops Something related to the skrub DataOps

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants