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

Use Tuple for parameters instead of Array #188

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

sathvikbhagavan
Copy link
Contributor

This is to avoid a performance warning:

https://github.com/SciML/SciMLBase.jl/blob/master/src/performance_warnings.jl#L12

As the parameters of the FMU can be of different types

@ThummeTo
Copy link
Owner

ThummeTo commented Aug 8, 2023

Dear @sathvikbhagavan,

I need to think about this... because parameters may change between simulation runs... or in theory also during simulation.
on the other hand, one could build a new ODEProblem for every run (as it its currently implemented)... I will check if this would work.

@ThummeTo ThummeTo merged commit 4d65a22 into ThummeTo:v0.13.0 Sep 19, 2023
0 of 15 checks passed
@CasBex
Copy link

CasBex commented Sep 22, 2023

I know I'm late to the party, but if I look at the SciMLBase code then the problems is not with having arrays SomeArray but with having arrays SomeArray{SomeAbstractType}. While having an empty tuple as default argument is certainly ok, I would still dispatch on p::Any for better flexibility (this is what DifferentialEquations.jl does I think). If users decide to use setupODEProblem manually for whatever reason, they will get the performance warning anyway and can decide for themselves to improve their code or not. If they don't everything is fine as well.

@ThummeTo
Copy link
Owner

Will try and apply ;)

ThummeTo added a commit that referenced this pull request Nov 7, 2023
* performance improvements

* refactor: use tuple for `p` instead of array (#188)

Co-authored-by: ThummeTo <[email protected]>

* further adjustments for FMISensitiviity.jl

* modifications for FMIsensitivity.jl

* rm restriction for cross chekcs

* reincluded load/save test because of JLD2 fix

* removed benchmark

---------

Co-authored-by: Sathvik Bhagavan <[email protected]>
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