-
Notifications
You must be signed in to change notification settings - Fork 174
[BREAKING] feat: update to SymbolicUtils@4 #1649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1846168 to
afede28
Compare
afede28 to
8a9a62c
Compare
a3b5441 to
f157562
Compare
6ad2d04 to
e01836d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, I left some questions
| using Symbolics.SymbolicUtils | ||
| using Symbolics: SymbolicT, VartypeT | ||
| using SymbolicUtils | ||
| using SymbolicUtils: symtype, unwrap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was doing using Symbolics.SymbolicUtils to laod SymbolicUtils from the Symbolics dependency. I think that was done so that we don't have both Symbolics & SymbolicUtils as extension triggers.
Would it make sense to split the extension or to have 2 triggers given that the SU part is now more significant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lux is the trigger, not Symbolics/SymbolicUtils. Dependencies of the parent package (Symbolics) are directly available to extensions without requiring a transitive import or additional triggers.
| function SymbolicUtils.promote_shape(f::LuxStatelessApplicator, x::SymbolicUtils.ShapeT, ::SymbolicUtils.ShapeT) | ||
| @nospecialize x ps | ||
| model = __get_model(f) | ||
| x = x::SymbolicUtils.ShapeVecT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do? 😅
Why don't we have x::SymbolicUtils.ShapeVecT in the function signature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
promote_shape takes the operation f along with the shapes of its arguments, and returns the shape of the result. The arguments are all annotated ::ShapeT because that is the type of the shape field in BasicSymbolic. Any call to promote_shape will at best infer ::ShapeT for the shape arguments. Annotating the argument with ::ShapeVecT would mean that the compiler doesn't know at compile time whether it can dispatch to this method or not. @nospecialize tells the compiler to not compile methods for more specific types of x and ps. This helps avoid dynamic dispatch.
b71cdc2 to
d523711
Compare
d523711 to
7ebe379
Compare
7ebe379 to
171aedc
Compare
Requires JuliaSymbolics/SymbolicUtils.jl#795
I haven't pushed test changes since they could need multiple rounds of updates, bloating the diff and making it harder to rebase.