Skip to content

Conversation

@baggepinnen
Copy link
Contributor

This PR adds the option to include the disturbance inputs as input arguments for the generated function. This is useful when designing state estimators where the noise model is non-additive.

@AayushSabharwal the PR includes a test, which is currently failing, likely because there is a modification to wrap_array_vars required. wrap_array_vars appears long and complicated without any comments or documentation, what does this function do and can it be made easier to extend?

Co-authored-by: Aayush Sabharwal <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@baggepinnen
Copy link
Contributor Author

The updates did not make the test pass. Looking at the generated function

julia> f[1]
RuntimeGeneratedFunction(#=in ModelingToolkit=#, #=using ModelingToolkit=#, :((ˍ₋arg1, ˍ₋arg2, ___mtkparameters___, ˍ₋arg4)->begin
          #= /home/fredrikb/.julia/packages/SymbolicUtils/jf8aQ/src/code.jl:385 =#
          #= /home/fredrikb/.julia/packages/SymbolicUtils/jf8aQ/src/code.jl:386 =#
          #= /home/fredrikb/.julia/packages/SymbolicUtils/jf8aQ/src/code.jl:387 =#
          begin
              begin
                  begin
                      begin
                          begin
                              #= /home/fredrikb/.julia/packages/SymbolicUtils/jf8aQ/src/code.jl:480 =#
                              (SymbolicUtils.Code.create_array)(typeof(ˍ₋arg1), nothing, Val{1}(), Val{(1,)}(), (+)((+)(ˍ₋arg2[1], (*)(-1, ˍ₋arg1[1])), (^)(ˍ₋arg4[1], 2)))
                          end
                      end
                  end
              end
          end
      end))

it looks like _arg4 is the disturbance_inputs, but time should have been _arg4 and the disturbance inputs should have been _arg5. Right now, time does not appear to be included among the inputs arguments

@baggepinnen
Copy link
Contributor Author

It turns out that it works correctly with split = false, but test failing with split=true

@AayushSabharwal
Copy link
Member

Ah, right cachesyms does that 😅. I'll push the required changes here.

@baggepinnen
Copy link
Contributor Author

looks good now, the test failures appear to be old

@AayushSabharwal
Copy link
Member

Yeah these failures are "expected". I have plans to fix two of them.

@ChrisRackauckas ChrisRackauckas merged commit 31f7a54 into master Dec 16, 2024
39 of 43 checks passed
@ChrisRackauckas ChrisRackauckas deleted the disturbance_args branch December 16, 2024 17:06
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