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 inner loop aggregation options #9

Merged
merged 6 commits into from
Aug 24, 2023

Conversation

JamesAllingham
Copy link
Contributor

@JamesAllingham JamesAllingham commented Aug 21, 2023

Fixes #8.

This PR introduces an aggregation option for the inner_loop callback, which controls how the logs from the inner loop are aggregated. The default behavior of returning the last value for each entry in a collection is maintained. This is equivalent to manually setting aggregation="last".

aggregation supports a string value, one of "last", "first", "min", "max", "mean", and "sum" or a Callable. In this case, all values in all collections will be aggregated as specified. However, since some collections might require different aggregations (e.g., "stateful_metrics" and "metrics" likely want "last" and "mean", respectively) it is also possible to specify a dict from collection to aggregation such as {"stateful_metrics": "last", "metrics": "mean"}. If a collection is not specified in this dictionary, then the "last" aggregation is used.

A inner_loop_kwargs argument has been added to train_loop in order to surface the aggregation options to a user of that API.

The PR also adds tests for both aggregation and inner_loop_kwargs.

Let me know what you think :) I also wasn't sure how best to document this functionality.

Also added a inner_loop_kwargs argument to train_loop.

Added tests for inner_loop aggregation and inner_loop_kwargs.
@JamesAllingham JamesAllingham changed the title Add inner loop agg Add inner loop aggregation options Aug 21, 2023
@@ -23,6 +23,9 @@
from ciclo.utils import get_batch_size, is_scalar


InnerLoopAggregation = Literal["last", "mean", "sum", "min", "max", "first"]
Copy link
Owner

Choose a reason for hiding this comment

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

Nice!

@cgarciae
Copy link
Owner

Thanks @JamesAllingham for doing this! I like the approach of transposing as it makes implementing reduction fairly easy.
I do have some request though, can you make it more general by also accepting Callables?

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #9 (c441c0f) into main (ce3a24e) will increase coverage by 0.48%.
Report is 3 commits behind head on main.
The diff coverage is 93.02%.

@@            Coverage Diff             @@
##             main       #9      +/-   ##
==========================================
+ Coverage   77.90%   78.39%   +0.48%     
==========================================
  Files          12       12              
  Lines        1539     1578      +39     
==========================================
+ Hits         1199     1237      +38     
- Misses        340      341       +1     
Files Changed Coverage Δ
ciclo/callbacks.py 63.69% <92.50%> (+3.92%) ⬆️
ciclo/__init__.py 100.00% <100.00%> (ø)
ciclo/loops/common.py 69.49% <100.00%> (+2.25%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@JamesAllingham
Copy link
Contributor Author

Thanks @JamesAllingham for doing this! I like the approach of transposing as it makes implementing reduction fairly easy. I do have some request though, can you make it more general by also accepting Callables?

No problem! Happy to contribute to this library. Thanks for the positive feedback.

That's a great idea – done. Let me know what you think of the changes.

Copy link
Owner

@cgarciae cgarciae left a comment

Choose a reason for hiding this comment

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

Thanks!

@JamesAllingham
Copy link
Contributor Author

My pleasure! Thanks for fixing the pre-commit.

@cgarciae
Copy link
Owner

Thanks @JamesAllingham for doing this!
Gave the PR a little push to pass it over the line.

@cgarciae cgarciae merged commit f39f265 into cgarciae:main Aug 24, 2023
9 checks passed
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.

inner_loop assumes the use of stateful metrics
2 participants