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] CX for each node #630

Merged
merged 68 commits into from
Apr 27, 2023
Merged

Conversation

AnaisFarr
Copy link
Contributor

@AnaisFarr AnaisFarr commented Mar 2, 2023

TODO BEFORE MERGING:

  • Launch big OCPs on bioptim main and this branch to compare the effect of the PR on memory.

All Submissions:

  • Have you followed the guidelines in our Contributing document [docs/contribution.md]?
  • Have you checked to ensure there aren't other open [Pull Requests] for the same update/change?
  • Have you opened/linked the issue related to your pull request?
  • Have you used the tag [WIP] for on-going changes, and removed it when the pull request was ready?
  • When ready to merge, have you sent a comment pinging @pariterre in it?

New Feature Submissions:

  1. Does your submission pass the tests (if not please explain why this is intended)?
  2. Did you write a proper documentation (docstrings and ReadMe)
  3. Have you linted your code locally prior to submission (using the command: black . -l120 --exclude "external/*")?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new examples for your core changes, as applicable?
  • Have you written new tests for your core changes, as applicable?

This change is Reviewable

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 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @AnaisFarr)


bioptim/examples/getting_started/robust_pendulum.py line 127 at r1 (raw file):

    ### New
    multinode_constraints = MultinodeConstraintList()

BinodeConstraint...

Code quote:

MultinodeConstraintList

bioptim/limits/multinode_constraint.py line 139 at r1 (raw file):

class AllnodeConstraint(Constraint):

AllNodeConstraint


bioptim/limits/multinode_constraint.py line 316 at r1 (raw file):

class AllnodeConstraintList(UniquePerPhaseOptionList):

AllNode...

Code quote:

AllnodeConstraintList

bioptim/limits/multinode_constraint.py line 355 at r1 (raw file):

        raise NotImplementedError("Printing of AllConstraintList is not ready yet")

    def prepare_allnode_constraints(self, ocp) -> list:

all_node

Code quote:

allnode

bioptim/optimization/optimization_variable.py line 407 at r1 (raw file):

        self.elements.append(value)

# Surement a modifier

This will create a bug in your tests because of the indentation

@pariterre pariterre mentioned this pull request Mar 2, 2023
@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Patch coverage: 77.37% and project coverage change: -0.47 ⚠️

Comparison is base (1f9a849) 81.79% compared to head (2cc7f29) 81.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #630      +/-   ##
==========================================
- Coverage   81.79%   81.33%   -0.47%     
==========================================
  Files         112      112              
  Lines       12558    12676     +118     
==========================================
+ Hits        10272    10310      +38     
- Misses       2286     2366      +80     
Impacted Files Coverage Δ
bioptim/gui/graph.py 95.18% <ø> (ø)
bioptim/limits/penalty_node.py 35.08% <0.00%> (ø)
...imal_control/double_pendulum_torque_driven_IOCP.py 37.06% <37.06%> (ø)
bioptim/gui/check_conditioning.py 85.53% <50.00%> (ø)
bioptim/limits/multinode_constraint.py 69.91% <56.52%> (-21.28%) ⬇️
bioptim/limits/penalty_option.py 85.85% <59.45%> (-4.26%) ⬇️
bioptim/limits/objective_functions.py 89.23% <65.51%> (+0.17%) ⬆️
...tim/examples/getting_started/example_multistart.py 81.13% <70.83%> (ø)
bioptim/optimization/multi_start.py 79.59% <72.22%> (ø)
bioptim/limits/constraints.py 81.71% <78.78%> (-1.68%) ⬇️
... and 37 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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 23 of 23 files at r3, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @AnaisFarr)


bioptim/limits/multinode_constraint.py line 368 at r3 (raw file):

                raise RuntimeError("Phase index of the allnode_constraint need to be positive")

            if mnc.weight: # ajouter un check, sinon mettre 1

Please write comments in english and/or do not commit non relevent comments

Code quote:

# ajouter un check, sinon mettre 1

bioptim/limits/multinode_constraint.py line 369 at r3 (raw file):

            if mnc.weight: # ajouter un check, sinon mettre 1
                mnc.base = ObjectiveFunction.LagrangeFunction # a elever

same

Code quote:

# a elever

bioptim/limits/penalty_option.py line 701 at r3 (raw file):

            # all_pn = []
            # # Make sure the penalty behave like a AllNodeConstraint, even though it may be an Objective or Constraint

Add TODO: clear this if not necessary anymore

Code quote:

            # all_pn = []
            # # Make sure the penalty behave like a AllNodeConstraint, even though it may be an Objective or Constraint
            # # self.transition = True
            # self.dt = 1
            # if not self.states_mapping:
            #     self.states_mapping = BiMapping(range(nlp.states.shape), range(nlp.states.shape))
            # nlp = ocp.nlp[self.phase_idx]
            #
            # all_pn.append()
            #
            # penalty_function = self.type(self, all_pn, **self.params)
            # self.set_penalty(penalty_function, all_pn)

bioptim/limits/phase_transition.py line 6 at r3 (raw file):

from casadi import vertcat, MX

from .multinode_constraint import BinodeConstraint, BinodeConstraintFunctions # AllNodeConstraint, AllNodeConstraintFunctions

Remove the comments if not necessary anymore


bioptim/optimization/optimization_variable.py line 321 at r3 (raw file):

                "OptimizationVariable must have been created by OptimizationVariableList to have a cx. "
            )
        return self.parent_list.cx_all[self.index, :]  #[self.index, :]

Remove that

Code quote:

#[self.index, :]

bioptim/optimization/optimization_variable.py line 366 at r3 (raw file):

        self._cx: MX | SX | np.ndarray = np.array([])
        self._cx_end: MX | SX | np.ndarray = np.array([])
        #self._cx_all: list = []

Remove that


bioptim/optimization/optimization_variable.py line 525 at r3 (raw file):

        """

        return self._cx_all

If this is already a property, make _cx_all public and remove the current property

Code quote:

self._cx_all

bioptim/examples/getting_started/example_binode_constraints.py line 209 at r3 (raw file):

    # sol.graphs()
    # --- Show results --- #
    #sol.animate()

Add a todo to restore that (or at least do not push these modification)

Code quote:

    # sol.graphs()
    # --- Show results --- #
    #sol.animate()

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 11 of 11 files at r4, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @AnaisFarr)


bioptim/dynamics/ode_solver.py line 77 at r4 (raw file):

        # nlp.dynamics = nlp.ode_solver.integrator(ocp, nlp)
        # if len(nlp.dynamics) != 1 and ocp.n_threads != 1:
        #     raise NotImplementedError("n_threads > 1 with external_forces is not implemented yet")

Add a TODO: verify if it is solved

Code quote:

raise NotImplementedError("n_threads > 1 with external_forces is not implemented yet")

bioptim/dynamics/ode_solver.py line 79 at r4 (raw file):

        #     raise NotImplementedError("n_threads > 1 with external_forces is not implemented yet")
        # if len(nlp.dynamics) == 1:
        #     nlp.dynamics = nlp.dynamics * nlp.ns

This can be removed

Code quote:

        # if len(nlp.dynamics) == 1:
        #     nlp.dynamics = nlp.dynamics * nlp.ns

bioptim/dynamics/ode_solver.py line 105 at r4 (raw file):

        self.defects_type = DefectType.NOT_APPLICABLE

    def integrator(self, ocp, nlp, node_index) -> list:

Don't forget to add this to other Solvers

Code quote:

, node_index

bioptim/dynamics/ode_solver.py line 158 at r4 (raw file):

        #     return dynamics_out
        # else:
        #     return [nlp.ode_solver.rk_integrator(ode, ode_opt)]

Do not forget to reintroduce with external forces

Code quote:

        # if len(nlp.external_forces) != 0:
        #     dynamics_out = []
        #     for idx in range(len(nlp.external_forces)):
        #         ode_opt["idx"] = idx
        #         dynamics_out.append(nlp.ode_solver.rk_integrator(ode, ode_opt))
        #     return dynamics_out
        # else:
        #     return [nlp.ode_solver.rk_integrator(ode, ode_opt)]

bioptim/dynamics/ode_solver.py line 300 at r4 (raw file):

            ode = {
                "x_unscaled": [nlp.states.cx[0]] + nlp.states.cx_intermediates_list,

index_node

Code quote:

[0]

bioptim/dynamics/ode_solver.py line 300 at r4 (raw file):

            ode = {
                "x_unscaled": [nlp.states.cx[0]] + nlp.states.cx_intermediates_list,

index_node also

Code quote:

cx_intermediates_list,

bioptim/examples/getting_started/example_multiphase.py line 31 at r4 (raw file):

def minimize_difference(all_pn: PenaltyNode):
    return all_pn[0].nlp.controls.cx[-1] - all_pn[1].nlp.controls.cx[0]

node_index= all_pn.node_index
all_pn[0].nlp.controls[node_index].cx_end

Code quote:

all_pn[0].nlp.controls.cx[-1]

bioptim/limits/multinode_constraint.py line 192 at r3 (raw file):

        if "force_allnode" in params:
            # This is a hack to circumvent the apparatus that moves the functions to a custom function
            # It is necessary for PhaseTransition

keep this unless it is now the vanilla way to do it

Code quote:

            # This is a hack to circumvent the apparatus that moves the functions to a custom function
            # It is necessary for PhaseTransition

bioptim/limits/multinode_constraint.py line 375 at r4 (raw file):

            if not mnc.weight: # ajouter un check, sinon mettre 1
                mnc.base = 1

verify what it is for

Code quote:

mnc.base = 1

bioptim/limits/penalty.py line 666 at r4 (raw file):

            nlp = all_pn.nlp
            if nlp.control_type == ControlType.CONSTANT:
                u = nlp.controls.cx[0]

0 should be node_index
nlp.controls[node_index].cx
nlp.controls[node_index].cx_end

Code quote:

 nlp.controls.cx[0]

bioptim/limits/phase_transition.py line 287 at r4 (raw file):

                    nlp_pre.states[key].mapping.to_second.map(nlp_pre.states[key].cx[-1]),
                )
                cx_start = vertcat(cx[0], nlp_post.states[key].mapping.to_second.map(nlp_post.states[key].cx[0]))

rewrite when ready

Code quote:

                    cx[-1],
                    nlp_pre.states[key].mapping.to_second.map(nlp_pre.states[key].cx[-1]),
                )
                cx_start = vertcat(cx[0], nlp_post.states[key].mapping.to_second.map(nlp_post.states[key].cx[0]))

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 29 of 29 files at r10, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @AnaisFarr)


bioptim/dynamics/ode_solver.py line 40 at r10 (raw file):

        self.is_direct_shooting = False

    def integrator(self, ocp, nlp, node_index=None) -> list:

Adde a TODO to remove that None

Code quote:

, node_index=None

tests/test_controltype_none.py line 126 at r9 (raw file):

        nlp.dynamics_func = Function(
            "ForwardDyn",
            [nlp.states[0]["scaled"].mx_reduced, nlp.controls[0]["scaled"].mx_reduced, nlp.parameters.mx],  # TODO: [0] to [node_index]

This TODO should be kept

Code quote:

 # TODO: [0] to [node_index]

tests/test_global_getting_started.py line 1281 at r10 (raw file):

    TestUtils.save_and_load(sol, ocp, True)

    # TODO: Restore that

I comment so I can see it later

Code quote:

 # TODO: Restore that

@AnaisFarr AnaisFarr changed the title [WIP] All nodes penalty (Add CX_all) [RTR] CX for each node Apr 25, 2023
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.

:lgtm:

Reviewed all commit messages.
Reviewable status: 7 of 63 files reviewed, 10 unresolved discussions (waiting on @AnaisFarr)

@pariterre pariterre merged commit 0ff87b3 into pyomeca:master Apr 27, 2023
4 of 5 checks passed
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.

None yet

3 participants