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

BVProblem with constraints #3323

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Conversation

vyudu
Copy link
Contributor

@vyudu vyudu commented Jan 14, 2025

Followup to #3251. Performance is something like 30x worse than generating the BVProblem using manually-defined f, bc, need to investigate why. Will uncomment the BVDAE solver tests when they get updated to use the same boundary condition format as the others.

@vyudu vyudu marked this pull request as draft January 14, 2025 19:07
@vyudu vyudu marked this pull request as ready for review January 15, 2025 22:22
@vyudu
Copy link
Contributor Author

vyudu commented Jan 15, 2025

I think this is mostly ready for review @ChrisRackauckas . Some of the problems in the tests don't seem fully well-posed, so I still need to work on that a bit, but the code itself should be correct.

Comment on lines +869 to +873
`constraints` are used to specify boundary conditions to the ODESystem in the
form of equations. These values should specify values that state variables should
take at specific points, as in `x(0.5) ~ 1`). More general constraints that
should hold over the entire solution, such as `x(t)^2 + y(t)^2`, should be
specified as one of the equations used to build the `ODESystem`. Below is an example.
Copy link
Member

Choose a reason for hiding this comment

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

It's odd that this is here. Problem constructors are only supposed to take values: constraints are structural. They should be added to the system I would think? @baggepinnen thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the 0.5 in x(0.5) ~ 1 be a parameter? I can imagine this being something one would like to change without re-simplifying the system.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have a BVPSystem that contains a ConstraintSystem like the OptimizationSystem?

Copy link
Member

Choose a reason for hiding this comment

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

Yes to both of these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I will work on these conversions.

Comment on lines +237 to +240
# Cartesian pendulum from the docs.
# DAE IVP solved using BoundaryValueDiffEq solvers.
# let
# @parameters g
Copy link
Member

Choose a reason for hiding this comment

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

What's up with these ones?

Copy link
Contributor Author

@vyudu vyudu Jan 22, 2025

Choose a reason for hiding this comment

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

The interface for specifying boundary conditions is different for the BVDAE solvers at the moment (you specify the time points as a separate argument), was gonna wait until they get updated to be the same.

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