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

[RTR/RTM] Algebraic states are states #907

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

Conversation

Ipuch
Copy link
Collaborator

@Ipuch Ipuch commented Jan 26, 2025

This PR ensures that algebraic states are treated as proper states in the optimization problem, which is particularly crucial for collocation-based transcription methods. Previously, algebraic states were handled inconsistently, leading to workarounds in various parts of the codebase. They were mainly controls.

🔹 Key Improvements:

  • Consistent Algebraic State Handling: Now properly integrated as decision variables in the OCP structure.
  • Enhanced Compatibility with Collocation Methods: Ensuring that algebraic states behave as expected within implicit dynamics formulations.
  • Partitioned Forward Dynamics Flexibility: Refactored partitioned_forward_dynamics to better support multi-phase problems.
  • Integration with Stochastic Models: Special care was taken to maintain compatibility with existing stochastic implementations, which previously relied on a non-standard approach. Preserving Stochastic Problem Implementations: Some stochastic formulations relied on unconventional structures, requiring additional handling to maintain backward compatibility see Stochastic examples with trapezoidal transcription doesnt give exactly the expected values #937.

Refactoring & Clean-Up: Standardized variable sizing across the codebase and improved test coverage for algebraic states.

This PR represents a major step forward in the formalization of algebraic states, particularly for advanced optimal control formulations that rely on collocations.


This change is Reviewable

@EveCharbie
Copy link
Collaborator

@Ipuch I repaired all the tests :)
If you are ready, you can change the status to RTR.

@Ipuch
Copy link
Collaborator Author

Ipuch commented Feb 28, 2025

That was quick, belle job ! 💪🏻

@Ipuch Ipuch changed the title [WIP] Algebraic states are states [RTR] Algebraic states are states Feb 28, 2025
Copy link
Member

@pariterre pariterre left a comment

Choose a reason for hiding this comment

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

Reviewed 15 of 23 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Ipuch)


bioptim/limits/constraints.py line 906 at r2 (raw file):

            # Algebraic states are pre-crunshed for each interval meaning the algebraic state cx_start in enough
            n_rep = controller.integrate_extra_dynamics(0).function.sparsity_in(4).shape[1]
            duplicated_algebraic_states = horzcat(*[controller.algebraic_states_scaled.cx_start for _ in range(n_rep)])

Is this true?


bioptim/dynamics/configure_new_variable.py line 355 at r2 (raw file):

                    all_variables_in_one_subplot = (
                        True if self.name in ["m", "cov", "k"] else False
                    )  # To Eve: This should not be there.

Remove this


bioptim/limits/penalty_controller.py line 131 at r2 (raw file):

        tp = OptimizationVariableList(self._nlp.cx, self._nlp.phase_dynamics == PhaseDynamics.SHARED_DURING_THE_PHASE)
        tp._cx_intermediates = [self._nlp.cx([])]

This should be done internally and not externally to make sure _cx_intermediate is in a usable state when constructed


bioptim/optimization/non_linear_program.py line 217 at r2 (raw file):

        self.integrated_values.initialize_intermediate_cx(
            n_shooting=self.ns + 1, n_cx=1
        )  # TODO #907 to confirm with Eve

Can remove the TODO


bioptim/optimization/optimal_control_program.py line 1616 at r2 (raw file):

        for i_phase, nlp in enumerate(self.nlp):
            numerical_timeseries += [OptimizationVariableList(self.cx, dynamics[i_phase].phase_dynamics)]
            numerical_timeseries[-1]._cx_intermediates = [self.cx([])]

Move this


bioptim/optimization/non_linear_program.py line 408 at r2 (raw file):

    @staticmethod
    def n_algebraic_states_decision_steps(node_idx) -> int:

def n_algebraic_states_decision_steps(node_idx) -> int:
return n_states_decision_steps(node_idx) -> int:


bioptim/optimization/optimization_vector.py line 86 at r2 (raw file):

                        )

                n_col = nlp.n_algebraic_states_decision_steps(k)

Revert this


bioptim/optimization/optimization_vector.py line 221 at r2 (raw file):

                nlp.a_bounds,
                nlp.a_scaling,
                lambda n: nlp.n_algebraic_states_decision_steps(n),

Revert this


bioptim/optimization/optimization_vector.py line 299 at r2 (raw file):

                nlp.a_init,
                nlp.a_scaling,
                lambda n: nlp.n_states_decision_steps(n),

algebraic_state


bioptim/optimization/optimization_vector.py line 423 at r2 (raw file):

            for node in range(nlp.n_states_nodes):
                nlp.algebraic_states.node_index = node
                n_cols = nlp.n_states_decision_steps(node)

algebraic_states


tests/utils.py line 215 at r2 (raw file):

    def initialize_numerical_timeseries(nlp, dynamics):
        numerical_timeseries = OptimizationVariableList(nlp.cx, dynamics.phase_dynamics)
        numerical_timeseries._cx_intermediates = [nlp.cx([])]

Move this


tests/shard4/test_penalty.py line 1324 at r2 (raw file):

    nlp.A_scaled = nlp.A
    tp = OptimizationVariableList(MX, phase_dynamics=phase_dynamics)
    tp._cx_intermediates = [MX()]

No need

Copy link
Collaborator

@EveCharbie EveCharbie left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Ipuch and @pariterre)


bioptim/limits/constraints.py line 906 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Is this true?

This was fixed in the other PR #933

Copy link
Collaborator Author

@Ipuch Ipuch left a comment

Choose a reason for hiding this comment

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

Reviewable status: 17 of 26 files reviewed, 12 unresolved discussions (waiting on @EveCharbie and @pariterre)


bioptim/dynamics/configure_new_variable.py line 355 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Remove this

@EveCharbie Est-ce qu'on a un moyen d'enlever ca ? je parle du all_variables_in_one_subplot


bioptim/limits/constraints.py line 906 at r2 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

This was fixed in the other PR #933

great !


bioptim/optimization/non_linear_program.py line 217 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Can remove the TODO

Done.


bioptim/optimization/non_linear_program.py line 408 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

def n_algebraic_states_decision_steps(node_idx) -> int:
return n_states_decision_steps(node_idx) -> int:

Done.


bioptim/optimization/optimal_control_program.py line 1616 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Move this

Done.


bioptim/optimization/optimization_vector.py line 86 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Revert this

Done.


bioptim/optimization/optimization_vector.py line 221 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Revert this

Done.


bioptim/optimization/optimization_vector.py line 299 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

algebraic_state

Done.


bioptim/optimization/optimization_vector.py line 423 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

algebraic_states

Done.


tests/utils.py line 215 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

Move this

Done.


tests/shard4/test_penalty.py line 1324 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

No need

Done.


bioptim/limits/penalty_controller.py line 131 at r2 (raw file):

Previously, pariterre (Pariterre) wrote…

This should be done internally and not externally to make sure _cx_intermediate is in a usable state when constructed

Done.

@Ipuch Ipuch changed the title Algebraic states are states [RTR/RTM] Algebraic states are states Mar 4, 2025
@Ipuch
Copy link
Collaborator Author

Ipuch commented Mar 4, 2025

@pariterre, we have passed through the whole chain of command, it's your turn for a final review or merge 👍

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