Skip to content

Conversation

@aplavin
Copy link
Member

@aplavin aplavin commented Jun 8, 2025

... also check whether tests fail on the upcoming julia version

@aplavin aplavin requested a review from Copilot June 8, 2025 23:24

This comment was marked as off-topic.

@aplavin
Copy link
Member Author

aplavin commented Jun 8, 2025

Indeed,

@test b_right.time b_default.time rtol=0.8
fails (composition order).
It appears to be a genuine regression in Julia. But also not sure what's the relationship of this test to Accessors, why L vs R composition orders are compared in the tests here... @jw3126 do you remember?

@jw3126
Copy link
Member

jw3126 commented Jun 9, 2025

A long time ago in julia a composed lens was fast or slow depending on the composition order. E.g. these two had different performance:

@optic(_.d)  (@optic(_.c)  (@optic(_.b)  @optic(_.a)))
((@optic(_.d)  @optic(_.c))  @optic(_.b))  @optic(_.a)

The purpose of the test was to check that @optic _.d.c.b.a gives us the fast one. Then julia improved and both orders became fast. Now it seems there is a regression and we again have a fast and a slow order.
Probably we should change the test to be:
@test min(b_left.time, b_right.time) ≈ b_default.time rtol=0.8 # no matter which order is faster, it is the default
E.g. we only care to get the fast one, but we don't really care in the test which one is the fast one.

@aplavin
Copy link
Member Author

aplavin commented Jun 9, 2025

Thanks for the background, updated the test accordingly and added a comment.

@aplavin aplavin merged commit 18e9ddb into master Jun 10, 2025
22 checks passed
@aplavin aplavin deleted the aplavin-patch-1 branch June 10, 2025 12:58
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