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 support for etime.Expt #124

Closed
wants to merge 1 commit into from
Closed

add support for etime.Expt #124

wants to merge 1 commit into from

Conversation

etuleu
Copy link
Contributor

@etuleu etuleu commented Jul 27, 2023

This is to enable stats for multiple runs == one experiment.

I am actually not sure why we had the LastNRows shortcut at the end so I removed it. But please let me know if we need that back. I tested this in boa and seems to work fine but not sure what the downside might be if any.

@etuleu etuleu requested a review from rcoreilly July 27, 2023 20:47
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01% 🎉

Comparison is base (2db038e) 35.13% compared to head (32d2fe6) 35.14%.

❗ Current head 32d2fe6 differs from pull request most recent head 0589e18. Consider uploading reports for the commit 0589e18 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
+ Coverage   35.13%   35.14%   +0.01%     
==========================================
  Files          80       80              
  Lines        6214     6211       -3     
==========================================
  Hits         2183     2183              
+ Misses       3824     3821       -3     
  Partials      207      207              
Files Changed Coverage Δ
elog/stditems.go 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rcoreilly
Copy link
Member

The LastNRows is for Run averaging over Epochs, and typically Run is the last one. I'll see about adding an option there.

@etuleu
Copy link
Contributor Author

etuleu commented Jul 27, 2023

So if we don't do the LastN call it averages over all epochs right? Isn't that fine? Maybe we can make the LastN thing a special case if we need it.

@rcoreilly
Copy link
Member

Yeah it is not ideal, but there are lots of calls to this method so I'm just going to push a version that always does the LastNRows thing for Run, and at least document that behavior. We could write an alternative version that doesn't do that, but in general you don't want to agg over all epochs because the start is learning, so in general this is better.

@rcoreilly rcoreilly closed this Jul 27, 2023
@etuleu etuleu deleted the etuleu/expt branch July 27, 2023 23:31
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

Successfully merging this pull request may close these issues.

2 participants