Add after method for drop(n).next(); easier for non-Scala folks - #3
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
It removes the existing LazyList.tapEvery extension used by tests (breaking compilation) and the top-level README example no longer compiles without importing deepwit.training.after.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an after(n) helper for training trajectories to replace the more idiomatic-but-less-obvious drop(n).next() pattern, and updates documentation/examples to use the new helper.
Changes:
- Add
Iterator.after(n)extension method indeepwit.training. - Update README/mdocs and multiple examples to use
.after(...)instead of.drop(...).next(). - Update imports in examples/mdocs to bring the
afterextension into scope.
File summaries
| File | Description |
|---|---|
| README.md | Switches documentation snippet to .after(numIterations) (currently missing the required import). |
| mdocs/README.md | Adds import deepwit.training.after and updates the compiled snippet to use .after(...). |
| examples/src/main/scala/deepwit/examples/variationalAutoencoder/VariationalAutoencoderTrain.scala | Imports after and replaces .drop(...).next() with .after(...). |
| examples/src/main/scala/deepwit/examples/thinning/MoonsMLPTrain.scala | Imports after and replaces .drop(...).next() with .after(...). |
| examples/src/main/scala/deepwit/examples/regression/Regression.scala | Imports after and replaces .drop(...).next() with .after(...). |
| examples/src/main/scala/deepwit/examples/neuralImage/NeuralImageTrain.scala | Imports after and replaces .drop(...).next() with .after(...). |
| examples/src/main/scala/deepwit/examples/mnistClassification/MNistCNNTrain.scala | Imports after and replaces .drop(...).next() with .after(...). |
| examples/src/main/scala/deepwit/examples/gpt/GPTTrain.scala | Imports after and replaces .drop(...).next() with .after(...). |
| examples/src/main/scala/deepwit/examples/autoencoder/AutoEncoderTrain.scala | Imports after and replaces .drop(...).next() with .after(...). |
| core/src/main/scala/deepwit/training/package.scala | Adds Iterator.after(n) but also removes LazyList.tapEvery (breaking existing tests/usages). |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2dafae5 to
7a1b80e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It includes an unadvertised breaking API removal (LazyList.tapEvery) and introduces Iterator.after without corresponding unit tests/docs being consistently copy-pasteable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
README.md:117
- This README snippet now uses
trainTrajectory.after(...), which requires importing the DeepWit extension method; unlikedrop(n).next()it won’t compile for readers as-is. Consider adding the needed import (or a short note) to keep the snippet copy/pasteable.
```scala
val finalState = trainTrajectory.after(numIterations)
core/src/main/scala/deepwit/training/package.scala:16
- This change removes the
LazyList.tapEveryextension (and its tests) fromdeepwit.training, which is a breaking public API change beyond the PR’s stated goal of addingafter. If LazyList support is still intended for library consumers, consider restoring it (or deprecating it first and documenting the removal).
/** The state after n iterations: Advances the iterator n steps and returns the resulting element */
def after(n: Int): T =
require(n >= 0, s"A number of steps must not be negative, but was $n.")
it.drop(n).next()
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
7a1b80e to
71f1af9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The README snippet now depends on importing the after extension (making it non-copy/paste-compilable as shown) and the PR also includes an uncalled-out breaking API removal (LazyList.tapEvery).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
README.md:117
- The README snippet now uses
trainTrajectory.after(numIterations), butafteris an extension method that requires an import (import deepwit.training.afterorimport deepwit.training.*). As written, the snippet won’t compile when copied on its own, whereas the previous.drop(...).next()did.
val finalState = trainTrajectory.after(numIterations)
core/src/main/scala/deepwit/training/package.scala:16
- This change also removes the
LazyList.tapEveryextension fromdeepwit.training, which is a source-compatible breaking API change for downstream code (even if it’s unused within this repo). If this wasn’t intentional, consider keeping it (possibly deprecated) rather than deleting it outright.
/** The state after n iterations: Advances the iterator n steps and returns the resulting element */
def after(n: Int): T =
require(n >= 0, s"A number of steps must not be negative, but was $n.")
it.drop(n).next()
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
No description provided.