Skip to content

Conversation

ErikQQY
Copy link
Collaborator

@ErikQQY ErikQQY commented Apr 26, 2025

Helps remove potential bugs and type instabilities

Copy link
Member

@penelopeysm penelopeysm left a comment

Choose a reason for hiding this comment

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

Generally, I prefer the explicit if... else... end expressions

Copy link
Member

@penelopeysm penelopeysm left a comment

Choose a reason for hiding this comment

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

Thank you for adding these :)

Is the failure on 1.12 expected?

@ErikQQY
Copy link
Collaborator Author

ErikQQY commented Apr 26, 2025

Yeah, the failures on 1.12 are expected

@yebai
Copy link
Member

yebai commented Apr 26, 2025

Thanks @ErikQQY and @penelopeysm. Please feel free to merge once you have two independent approvals!

@ErikQQY ErikQQY merged commit a8c0ee7 into main Apr 27, 2025
14 of 17 checks passed
@ErikQQY ErikQQY deleted the qqy/quality branch April 27, 2025 09:54
@@ -218,7 +218,7 @@ function step(
res = if FullTraj
Vector{P}(undef, n_steps)
else
z
Vector{P}(undef, 1)
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed? This creates unnecessary allocations in every step. There's no type inference issue here since FullTraj is a type parameter of a Val, so the compiler compiles and optimizes separate methods for FullTraj = true and FullTraj = false where these branches are eliminated.

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.

4 participants