Skip to content

Patch optimizer rework - #143

Merged
benikm91 merged 2 commits into
dimwit-dev:mainfrom
benikm91:rework-optimizer-patch
Aug 11, 2026
Merged

Patch optimizer rework#143
benikm91 merged 2 commits into
dimwit-dev:mainfrom
benikm91:rework-optimizer-patch

Conversation

@benikm91

Copy link
Copy Markdown
Collaborator

I found three issues when integrating new optimizers into the GPT example...

  1. LearningRateSchedule should be renamed to LearningRateScheduler

  1. Adam constructor was buggy
class Adam[V: IsFloating](
    val learningRate: Tensor0[V],
    b1: Tensor0[V] = Tensor0(0.99), // This hard coded V as Float64, if not using Adam.of
    b2: Tensor0[V],
    epsilon: Tensor0[V] 

Solution: Remove default values and have a constructor with Double for the common case.


  1. AdamState having V makes handling this state unnecessarily hard. I suggest hard-coding the beta to store and load as Float32.
case class TrainingState(
  params: GPT.Params[Float32],
  optState: LearningRateSchedulerState[GPT.Params[Float32], [P] =>> AdamState[P, Float32]], // <--
)

vs.

case class TrainingState(
  params: GPT.Params[Float32],
  optState: LearningRateSchedulerState[GPT.Params[Float32], AdamState], // <--
)

I hope this fixes it enough that we don't run into more issues.

@benikm91
benikm91 requested a review from marcelluethi August 10, 2026 11:17
(1) FloatTree was a bad name for this abstraction as it did not require V to be a Floating, renamed to TreeOf and implemented correctly: If no float required V; if float required V: IsFloating. Added test for TreeOf with Int32

(2) Update optimizers changing constructor values to Tensor0[Float32] moving precision to init and update method making precision depend on params. Optimizers don't carry a precision, they adapt to the parameters' precision.
@benikm91
benikm91 force-pushed the rework-optimizer-patch branch from dfe3078 to 464609b Compare August 11, 2026 12:42

@marcelluethi marcelluethi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Thanks for fixing it.

@benikm91
benikm91 merged commit 75f6499 into dimwit-dev:main Aug 11, 2026
1 check 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.

2 participants