Skip to content
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

Relax type constraints to allow callable parameters in pdeps #3079

Merged
merged 4 commits into from
Oct 5, 2024

Conversation

SebastianM-C
Copy link
Contributor

@SebastianM-C SebastianM-C commented Sep 26, 2024

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Currently parameter dependencies assume that we only have BasicSymbolic. I tried to relax that constraint a bit to allow callable parameters to be stored too. If one has both callable parameters and normal ones in the parameter dependencies, I think that there might be a performance issue, but otherwise it should not affect nortmal usage.

@AayushSabharwal is this the right way of handling this issue? I think that for normal parametrs we store things in the non-numeric portion and that's splitted by types (?), so that might be better for the heterogenuous case.

@SebastianM-C
Copy link
Contributor Author

I'm having some trouble with the test. The codegen doesn't seem to do the replacement of the parameter in prob.f. In the function it does ˍ₋arg2[1], but in the pdep call it's still CallableFoo(p).

julia> prob.f.f.f_iip
RuntimeGeneratedFunction(#=in ModelingToolkit=#, #=using ModelingToolkit=#, :((ˍ₋out, ˍ₋arg1, ˍ₋arg2, t)->begin    
          #= C:\Users\sebastian\.julia\packages\SymbolicUtils\ij6YM\src\code.jl:385 =#
          #= C:\Users\sebastian\.julia\packages\SymbolicUtils\ij6YM\src\code.jl:386 =#
          #= C:\Users\sebastian\.julia\packages\SymbolicUtils\ij6YM\src\code.jl:387 =#
          begin
              begin
                  begin
                      i = CallableFoo(p)
                      begin
                          #= C:\Users\sebastian\.julia\packages\Symbolics\b4I7P\src\build_function.jl:546 =#       
                          #= C:\Users\sebastian\.julia\packages\SymbolicUtils\ij6YM\src\code.jl:434 =# @inbounds begin
                                  #= C:\Users\sebastian\.julia\packages\SymbolicUtils\ij6YM\src\code.jl:430 =#     
                                  ˍ₋out[1] = (+)(ˍ₋arg2[1], i(t))
                                  #= C:\Users\sebastian\.julia\packages\SymbolicUtils\ij6YM\src\code.jl:432 =#     
                                  nothing
                              end
                      end
                  end
              end
          end
      end))

@AayushSabharwal
Copy link
Member

but in the pdep call it's still CallableFoo(p).

You probably need to @register_symbolic CallableFoo(p)

@SebastianM-C SebastianM-C force-pushed the smc/pdeps branch 2 times, most recently from 2526813 to 89886a0 Compare October 4, 2024 15:25
@SebastianM-C
Copy link
Contributor Author

Thanks! That fixed it.

@SebastianM-C
Copy link
Contributor Author

@ChrisRackauckas test failures seem the same as master. Does this look okay?

@ChrisRackauckas
Copy link
Member

I don't like that they are just relaxed, but if @AayushSabharwal thinks it's required then let's go with it.

@SebastianM-C
Copy link
Contributor Author

What would be the alternative? I understand that we don't want to introduce type parameters (to avoid increasing inference time?).

@AayushSabharwal
Copy link
Member

I don't like that they are just relaxed

What's not to like? 😅 Looks good to me, and a fairly simple change allows new features.

@ChrisRackauckas
Copy link
Member

    lhss = BasicSymbolic[]
    lhss = []

lost a validation step, but if it's required, sure.

@ChrisRackauckas ChrisRackauckas merged commit 2497d9b into SciML:master Oct 5, 2024
21 of 25 checks passed
@SebastianM-C SebastianM-C deleted the smc/pdeps branch October 5, 2024 16:54
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