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

ODEProblem Parameter type for driving models with data #2220

Closed
wants to merge 9 commits into from

Conversation

bradcarman
Copy link
Contributor

@bradcarman bradcarman commented Aug 4, 2023

Currently using the parameter space for a mix of single values and vectors is not so easy using ModelingToolkit. See the SampleData component for example (https://docs.sciml.ai/ModelingToolkitStandardLibrary/stable/tutorials/input_component/). The code for this component needs to explicitly convert the parameter vector to the Parameter type for the SampleData component and remake the ODEProblem...

    p = Parameter.(ModelingToolkit.varmap_to_vars(defs, parameters(sys); tofloat = false))
    remake(prob; p)

This code is now simply...

prob = ODEProblem(sys, [], (0.0, t_end))

To achieve this the Parameter{T} type is moved from ModelingToolkitStandardLibrary.jl to ModelingToolkit.jl

Copy link

@ai-maintainer ai-maintainer bot left a comment

Choose a reason for hiding this comment

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

AI-Maintainer Review for PR - ODEProblem p_type keyword for parameter element type specification

Title and Description 👍

The Title and description are clear, concise and helpful
The title and description of the pull request are clear and concise. They effectively communicate the purpose of the changes, which is to add a new keyword argument `p_type` to the `ODEProblem` constructor for specifying the element type of the parameters. The description provides a good context for the changes and explains the problem that these changes aim to solve.

Scope of Changes 👍

The changes are narrowly focused
The changes in this pull request are narrowly focused on a specific issue. The addition of the `p_type` keyword argument to the `ODEProblem` constructor is aimed at addressing the challenge of using the parameter space for a mix of single values and vectors in ModelingToolkit. The diff primarily consists of modifications to a few specific functions and classes related to handling initial conditions and parameters in differential equation problems.

Testing 👎

Testing details are missing
The description does not mention how the author tested the changes. It is common practice for authors to include information about how they tested their changes in the pull request description. This could include details about the test environment, specific test cases used, or any relevant test results. Without this information, it is unclear how the author ensured the correctness and effectiveness of the changes.

Documentation 👎

Docstrings are missing for some functions
Based on the provided diff, the following functions, classes, or methods have been added and do not have docstrings:
  • promote_to_concrete in src/utils.jl
  • varmap_to_vars in src/variables.jl

These additions should have docstrings added to describe their behavior, arguments, and return values.

Suggested Changes

  • Please add docstrings to the promote_to_concrete function in src/utils.jl and the varmap_to_vars function in src/variables.jl to describe their behavior, arguments, and return values.
  • Please provide details about how you tested the changes. This could include information about the test environment, specific test cases used, or any relevant test results.

Reviewed with AI Maintainer

@bradcarman bradcarman changed the title ODEProblem p_type keyword for parameter element type specification ODEProblem Parameter type for driving models with data Aug 8, 2023
@YingboMa
Copy link
Member

This is superseded by #2231

@YingboMa YingboMa closed this Sep 21, 2023
@YingboMa YingboMa deleted the bgc/p_type branch September 21, 2023 16:33
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.

2 participants