Skip to content

Fix dtype casting inside tree_add_scale. - #1376

Merged
copybara-service[bot] merged 1 commit into
google-deepmind:mainfrom
carlosgmartin:tree_add_scale_dtype
Sep 8, 2025
Merged

Fix dtype casting inside tree_add_scale.#1376
copybara-service[bot] merged 1 commit into
google-deepmind:mainfrom
carlosgmartin:tree_add_scale_dtype

Conversation

@carlosgmartin

@carlosgmartin carlosgmartin commented Jul 11, 2025

Copy link
Copy Markdown
Contributor

The current behavior of tree_add_scale is confusing:

$ py -c "import optax; print(optax.tree_utils.tree_add_scale([1], 0.1, [1]))"
[Array(1, dtype=int32)]

This is because its current implementation casts scalar to the dtype of x.

This PR modifies tree_add_scale to remove this casting and thus get the expected behavior:

$ py -c "import optax; print(optax.tree_utils.tree_add_scale([1], 0.1, [1]))"
[Array(1.1, dtype=float32, weak_type=True)]

Subsequently, it fixes the scan loop bodies inside _precondition_by_lbfgs to use tree_cast_like to ensure the carry output matches the carry input.

@vroulet

vroulet commented Jul 15, 2025

Copy link
Copy Markdown
Collaborator

@mtthss you introduced this dtype conversion in #926. Do you remember why?

@carlosgmartin add a test. It's clearly a bug so a test is welcome. Thanks again!

@vroulet

vroulet commented Jul 15, 2025

Copy link
Copy Markdown
Collaborator

Also @carlosgmartin, can you explain

  1. the bug that you then get with lbfgs
  2. whether it was caught by a test or whether you found it yourself

There may be other functions that work well thanks to this undocumented dtype conversion. So maybe we'll have to make a pass on references to tree_add_scale

@carlosgmartin
carlosgmartin force-pushed the tree_add_scale_dtype branch from 9c4d1cc to 029f100 Compare July 15, 2025 16:53
@carlosgmartin
carlosgmartin force-pushed the tree_add_scale_dtype branch from 029f100 to 7cf6896 Compare July 15, 2025 16:55
@carlosgmartin

Copy link
Copy Markdown
Contributor Author

@vroulet The induced LBFGS bug was caught by a test. Without the two

vec_new = optax.tree.cast_like(vec_new, vec)

lines,

$ pytest optax/_src/alias_test.py -k test_state_shape_dtype_shard_stability

outputs

optax/_src/alias_test.py ..s..........s..ss...........ss...........ss.....ss [ 25%]
.................s......ss..s........ss.....ss.............................. [ 62%]
..........................ss...........ss..................ss....FF..FF..... [ 99%]
.                                                                            [100%]

===================================== FAILURES =====================================
________________ LBFGSTest.test_state_shape_dtype_shard_stability0 _________________

self = <alias_test.LBFGSTest testMethod=test_state_shape_dtype_shard_stability0>
dtype = <class 'jax.numpy.float16'>
linesearch = GradientTransformationExtraArgs(init=<function scale_by_backtracking_linesearch.<locals>.init_fn at 0x127a46980>, update=<function scale_by_backtracking_linesearch.<locals>.update_fn at 0x127a46b60>)

...

E       TypeError: scan body function carry input and carry output must have equal types, but they differ:
E       
E       The input carry vec has type float16[3] but the corresponding output carry component has type float32[3], so the dtypes do not match.
E       
E       Revise the function so that all output types match the corresponding input types.

...

chex.assert_trees_all_close(expected, got)

def test_tree_add_scale_dtype(self):
got = tu.tree_add_scale(3, 0.1, 2)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove this test please

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@vroulet asked me to add a test here. Can you clarify?

@rdyro

rdyro commented Jul 15, 2025

Copy link
Copy Markdown
Collaborator

Removing the dtype cast in tree_add_scale is a good idea and making this explicit for L-BFGS is great! Thanks @carlosgmartin

@copybara-service
copybara-service Bot merged commit 46411b5 into google-deepmind:main Sep 8, 2025
15 checks passed
@carlosgmartin
carlosgmartin deleted the tree_add_scale_dtype branch September 8, 2025 22:56
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.

3 participants