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

[RTM] Sample data control #628

Merged
merged 33 commits into from
Mar 30, 2023
Merged

Conversation

Kev1CO
Copy link
Collaborator

@Kev1CO Kev1CO commented Mar 1, 2023

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

Kev1CO and others added 12 commits February 10, 2023 15:51
integrators for sampleddataOCP
blocked at return x_prev + h * self.fun(x_prev, self.get_u(u, t), p)[:, self.idx] because RuntimeError: D:\bld\casadi_1669335641627\work\casadi\core\sparsity_internal.cpp:2151: Assertion "in_range(cc, -size2()+ind1, size2()+ind1)" failed:
Out of bounds error. Got elements in range [1,1], which is outside the range [-1,1). Can't find the solution... yet !
graph values are still incorrect
@Kev1CO
Copy link
Collaborator Author

Kev1CO commented Mar 1, 2023

This pull request is to allow the execution in Bioptim of Pontryagin : optimal sampled-data control

@Kev1CO Kev1CO changed the title Sample_data_control [WIP] Sample data control Mar 1, 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.

Reviewed 5 of 10 files at r3, 9 of 9 files at r5, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Kev1CO)


bioptim/dynamics/integrator.py line 76 at r5 (raw file):

            self.u_sym = ode["p_scaled"]
        else:
            self.u_sym = []

Please use ternary operator for these trivial tests
e.g. self.u_sym = ode["p_scaled"] if "p_scale" in ode.keys() else []

Also unless it makes sense to do so, prefer None to []

Code quote:

        if "p_scale" in ode.keys():
            self.u_sym = ode["p_scaled"]
        else:
            self.u_sym = []

bioptim/dynamics/ode_solver.py line 128 at r5 (raw file):

            "defects_type": DefectType.NOT_APPLICABLE,
        }
        if nlp.controls.shape != 0:

Try to avoid "not" operator when possible in if statements, unless it avoids the else completely. It is far more easier to understand if something is ..., then ... otherwise ..., than to understand if something is not ..., then ... otherwise ....

Code quote:

 if nlp.controls.shape != 0:

bioptim/dynamics/ode_solver.py line 146 at r5 (raw file):

                "x_scaled": nlp.states["scaled"].cx,
                "ode": nlp.dynamics_func,
                "implicit_ode": nlp.implicit_dynamics_func,

Providing different data structure is generally bad practise as it leads to having to test if some key exists.
A better practise is to make sure it is a valid structure, but filled with None for the non-sensical one.

Code quote:

        else:
            ode = {
                "x_unscaled": nlp.states.cx,
                "x_scaled": nlp.states["scaled"].cx,
                "ode": nlp.dynamics_func,
                "implicit_ode": nlp.implicit_dynamics_func,
            }

bioptim/dynamics/ode_solver.py line 155 at r5 (raw file):

                dynamics_out.append(nlp.ode_solver.rk_integrator(ode, ode_opt))
            return dynamics_out
        elif isinstance(ode["ode"], list):

This elif does not oppose if len(nlp.external_forcesin any meaningful way. both can be true and therefore your piece of code will never be run.

Also do not test for instance unless you are trying to differentiate inherited elements. Use a flag instead.

Code quote:

elif isinstance(ode["ode"], list):

bioptim/interfaces/solve_ivp_interface.py line 350 at r5 (raw file):

    for s, func in enumerate(dynamics_func):
        u_slice = slice(s, s + 1) if control_type == ControlType.CONSTANT else slice(s, s + 2)
        u_controls = [] if not u else u[:, u_slice]

if u, instead of not u. Also, u should be a None, not an empty list. So if u is None is a better option

Code quote:

[] if not u else u[:, u_slice]

bioptim/limits/penalty.py line 676 at r5 (raw file):

            penalty.expand = all_pn.nlp.dynamics_type.expand

Please add a raise here (just prior to that line) that raises a NotImplementedYet if len(penalty.node_idx) > 1
(PS I know you are not the one who wrote that piece of code)

Code quote:

node_idx = penalty.node_idx[0] if len(penalty.node_idx) == 1 else 0

bioptim/limits/penalty_option.py line 427 at r5 (raw file):

        param_cx = nlp.cx(nlp.parameters.cx)
        if isinstance(nlp.controls.cx, list):

Do not test for instance unless it makes sense. If you used a None, then test for None

In other word, avoid encoding the information that something means something else because it is empty.

Code quote:

isinstance(nlp.controls.cx, list)

bioptim/optimization/optimal_control_program.py line 513 at r5 (raw file):

            self.nlp[i].initialize(self.cx)
            ConfigureProblem.initialize(self, self.nlp[i])
        for i in range(self.n_phases):

Why do you need two for loops for that?
please add a comment in the second to quickly explain why

Code quote:

 for i in range(self.n_phases):

bioptim/optimization/optimization_variable.py line 483 at r5 (raw file):

        """

        return self._cx[:, 0] if self.shape != 0 else []

This should be CX() and not a list OR a None

Also do not test negative when there is an else

Code quote:

[]

bioptim/optimization/optimization_variable.py line 491 at r5 (raw file):

        """

        return self._cx_end[:, 0] if self.shape != 0 else []

This should be CX() and not a list OR a None

Also do not test negative when there is an else

Code quote:

 if self.shape != 0 else []

bioptim/optimization/optimization_vector.py line 312 at r5 (raw file):

        p_idx = 0

        if self.n_all_u != 0:

Are you sure that this loop does not already do what you want? I feel that if data are empty, then the loop will just be skipped anyway (as dimensions will be 0). It seems redundant and unnecessarily complex...

Code quote:

if self.n_all_u != 0:

bioptim/optimization/solution.py line 1017 at r5 (raw file):

            param_scaling = nlp.parameters.scaling
            x0 = self._get_first_frame_states(out, shooting_type, phase=p)
            u = np.array([]) if nlp.controls.shape == 0 else self._controls["unscaled"][controls_phase_idx]["all"]

I feel this should be a None... but maybe it causes problem

Code quote:

 np.array([])

Copy link
Collaborator Author

@Kev1CO Kev1CO 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: 4 of 16 files reviewed, 12 unresolved discussions (waiting on @pariterre)


bioptim/dynamics/integrator.py line 76 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

Please use ternary operator for these trivial tests
e.g. self.u_sym = ode["p_scaled"] if "p_scale" in ode.keys() else []

Also unless it makes sense to do so, prefer None to []

Done.
Not able to put None as the program needs a list, even empty


bioptim/dynamics/ode_solver.py line 128 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

Try to avoid "not" operator when possible in if statements, unless it avoids the else completely. It is far more easier to understand if something is ..., then ... otherwise ..., than to understand if something is not ..., then ... otherwise ....

Done.


bioptim/dynamics/ode_solver.py line 146 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

Providing different data structure is generally bad practise as it leads to having to test if some key exists.
A better practise is to make sure it is a valid structure, but filled with None for the non-sensical one.

Done.


bioptim/dynamics/ode_solver.py line 155 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

This elif does not oppose if len(nlp.external_forcesin any meaningful way. both can be true and therefore your piece of code will never be run.

Also do not test for instance unless you are trying to differentiate inherited elements. Use a flag instead.

Ode is either a function or a list of function with our case, this is the reason why we used a isinstance condition to verify our problem
not sure to understant the use of flag instead of isinstance


bioptim/interfaces/solve_ivp_interface.py line 350 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

if u, instead of not u. Also, u should be a None, not an empty list. So if u is None is a better option

Done.
Not able to put None as the program needs a list, even empty


bioptim/limits/penalty.py line 676 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

Please add a raise here (just prior to that line) that raises a NotImplementedYet if len(penalty.node_idx) > 1
(PS I know you are not the one who wrote that piece of code)

Done.


bioptim/limits/penalty_option.py line 427 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

Do not test for instance unless it makes sense. If you used a None, then test for None

In other word, avoid encoding the information that something means something else because it is empty.

Test is mendatory, cannot use none as the solver needs a list


bioptim/optimization/optimal_control_program.py line 513 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

Why do you need two for loops for that?
please add a comment in the second to quickly explain why

Forgotten modification for tests
Done.


bioptim/optimization/optimization_variable.py line 483 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

This should be CX() and not a list OR a None

Also do not test negative when there is an else

Can't


bioptim/optimization/optimization_variable.py line 491 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

This should be CX() and not a list OR a None

Also do not test negative when there is an else

Can't


bioptim/optimization/optimization_vector.py line 312 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

Are you sure that this loop does not already do what you want? I feel that if data are empty, then the loop will just be skipped anyway (as dimensions will be 0). It seems redundant and unnecessarily complex...

No this flag is necessary otherwise it will go trought and try to slice a empty control vector


bioptim/optimization/solution.py line 1017 at r5 (raw file):

Previously, pariterre (Pariterre) wrote…

I feel this should be a None... but maybe it causes problem

Yes it does if not np.array([])

@Kev1CO Kev1CO changed the title [WIP] Sample data control [RTR] Sample data control Mar 2, 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.

Reviewed 3 of 12 files at r6, 3 of 9 files at r7, 5 of 7 files at r8, all commit messages.
Reviewable status: 14 of 16 files reviewed, 15 unresolved discussions (waiting on @Kev1CO)


bioptim/dynamics/integrator.py line 73 at r8 (raw file):

        self.cx = ode_opt["cx"]
        self.x_sym = ode["x_scaled"]
        self.u_sym = ode["p_scaled"] if "p_scale" in ode.keys() else []

This should be useless

Code quote:

 if "p_scale" in ode.keys() else

bioptim/dynamics/integrator.py line 300 at r8 (raw file):

        """

        return x_prev + h * self.fun(x_prev, self.get_u(u, t), p)  # [:, self.idx]

Why did you remove "[:, idx]"?

Code quote:

# [:, self.idx]

bioptim/dynamics/ode_solver.py line 128 at r8 (raw file):

            "defects_type": DefectType.NOT_APPLICABLE,
        }
        if nlp.controls.shape == 0:

This if should be be element, so we are sure we have the same dict

Code quote:

if nlp.controls.shape == 0:

bioptim/dynamics/ode_solver.py line 133 at r8 (raw file):

                "x_scaled": nlp.states["scaled"].cx,
                "p_unscaled": None,
                "p_scaled": None,

p_scaled: None if nlp.controls.shape == 0 else nlp.controls["scaled"]

Change to MX()

Code quote:

"p_scaled": None,

bioptim/dynamics/ode_solver.py line 333 at r8 (raw file):

            if len(nlp.external_forces) == 0:
                return [nlp.ode_solver.rk_integrator(ode, ode_opt)]
            else:

No need for else (as it already returns)

Code quote:

else:

Copy link
Collaborator

@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.

Reviewed 3 of 12 files at r6, 1 of 9 files at r7, 3 of 7 files at r8, all commit messages.
Reviewable status: 15 of 16 files reviewed, 15 unresolved discussions (waiting on @Kev1CO and @pariterre)


bioptim/dynamics/ode_solver.py line 155 at r5 (raw file):

Previously, Kev1CO wrote…

Ode is either a function or a list of function with our case, this is the reason why we used a isinstance condition to verify our problem
not sure to understant the use of flag instead of isinstance

We need a more META way to treat this information.
Instead of sending list of dynamics we need to have a MX of size n x n_shooting nodes as we have for external forces. So there will be only one if case.


bioptim/limits/penalty_option.py line 427 at r5 (raw file):

Previously, Kev1CO wrote…

Test is mendatory, cannot use none as the solver needs a list

We need a need feature, global flag to test this:
"ControlType.NONE"


bioptim/optimization/optimization_variable.py line 483 at r5 (raw file):

Previously, Kev1CO wrote…

Can't

Let's try again together.


bioptim/optimization/optimization_variable.py line 491 at r5 (raw file):

Let's try again together.


bioptim/optimization/optimization_vector.py line 312 at r5 (raw file):

Previously, Kev1CO wrote…

No this flag is necessary otherwise it will go trought and try to slice a empty control vector

ControlType.NONE


bioptim/optimization/solution.py line 1017 at r5 (raw file):

Previously, Kev1CO wrote…

Yes it does if not np.array([])

ControlType.None

Code quote:

nlp.controls.shape == 0

@Kev1CO Kev1CO changed the title [RTR] Sample data control [WIP] Sample data control Mar 8, 2023
Copy link
Collaborator Author

@Kev1CO Kev1CO 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: 15 of 16 files reviewed, 15 unresolved discussions (waiting on @pariterre)


bioptim/dynamics/integrator.py line 73 at r8 (raw file):

if "p_scale" in ode.keys() else
When there is no control, there is no "p_scale" in the ode
That is why we keep it


bioptim/dynamics/integrator.py line 300 at r8 (raw file):

Previously, pariterre (Pariterre) wrote…

Why did you remove "[:, idx]"?

Cannot remember, works well with it anyway
Done.


bioptim/dynamics/ode_solver.py line 128 at r8 (raw file):

Previously, pariterre (Pariterre) wrote…

This if should be be element, so we are sure we have the same dict

Done.


bioptim/dynamics/ode_solver.py line 133 at r8 (raw file):

Previously, pariterre (Pariterre) wrote…

p_scaled: None if nlp.controls.shape == 0 else nlp.controls["scaled"]

Change to MX()

Done.


bioptim/dynamics/ode_solver.py line 333 at r8 (raw file):

Previously, pariterre (Pariterre) wrote…

No need for else (as it already returns)

Done.

Code quote:

                dynamics_out = []
                for idx in range(len(nlp.external_forces)):
                    ode_opt["idx"] = idx

Kevin CO and others added 2 commits March 7, 2023 19:32
Ode and integrator modification for sample_data_control PR
Copy link
Collaborator

@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: 13 of 16 files reviewed, 15 unresolved discussions (waiting on @pariterre)


bioptim/dynamics/ode_solver.py line 155 at r5 (raw file):

len(nlp.external_forces)
up

Copy link
Collaborator Author

@Kev1CO Kev1CO 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: 10 of 17 files reviewed, 15 unresolved discussions (waiting on @Ipuch, @Kev1CO, and @pariterre)


bioptim/optimization/optimization_vector.py line 312 at r5 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

ControlType.NONE

DONE.


bioptim/optimization/solution.py line 1017 at r5 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

ControlType.None

DONE.

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 1 of 7 files at r8, 1 of 5 files at r10, 6 of 6 files at r11, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Kev1CO)


bioptim/dynamics/integrator.py line 73 at r8 (raw file):

Previously, Kev1CO wrote…

if "p_scale" in ode.keys() else
When there is no control, there is no "p_scale" in the ode
That is why we keep it

self.u_sym = [] if ode_opt["control_type"] is ControlType.NONE else ode["p_scaled"]


bioptim/dynamics/integrator.py line 122 at r11 (raw file):

            return u[:, 0] + (u[:, 1] - u[:, 0]) * dt_norm
        elif self.control_type == ControlType.NONE:
            return np.ndarray([])

either:
np.ndarray(0) or np.array(())

Code quote:

np.ndarray([])

bioptim/dynamics/ode_solver.py line 132 at r11 (raw file):

            "x_unscaled": nlp.states.cx,
            "x_scaled": nlp.states["scaled"].cx,
            "p_unscaled": MX() if nlp.controls.shape == 0 else nlp.controls.cx

change this for ControlType

Code quote:

nlp.controls.shape

bioptim/dynamics/ode_solver.py line 135 at r11 (raw file):

            if nlp.control_type == ControlType.CONSTANT
            else horzcat(nlp.controls.cx, nlp.controls.cx_end),
            "p_scaled": MX() if nlp.controls.shape == 0 else nlp.controls["scaled"].cx

Use ControlType

Code quote:

nlp.controls.shape

bioptim/limits/penalty.py line 670 at r11 (raw file):

                u = horzcat(nlp.controls.cx, nlp.controls.cx_end)
            elif nlp.control_type == ControlType.NONE:
                u = []

This should be a CX...

Code quote:

[]

bioptim/optimization/optimization_vector.py line 453 at r11 (raw file):

            # For controls
            if nlp.use_controls_from_phase_idx == nlp.phase_idx:
                if nlp.control_type == ControlType.CONSTANT or nlp.control_type == ControlType.NONE:

nlp.control_type in (ControlType.CONSTANT, ControlType.NONE)

Code quote:

nlp.control_type == ControlType.CONSTANT or nlp.control_type == ControlType.NONE

bioptim/optimization/optimization_vector.py line 508 at r11 (raw file):

            if nlp.use_controls_from_phase_idx == nlp.phase_idx:
                if nlp.control_type == ControlType.CONSTANT or nlp.control_type == ControlType.NONE:

nlp.control_type in (ControlType.CONSTANT, ControlType.NONE)

Code quote:

nlp.control_type == ControlType.CONSTANT or nlp.control_type == ControlType.NONE

bioptim/optimization/optimization_vector.py line 544 at r11 (raw file):

            # For controls
            if nlp.use_controls_from_phase_idx == nlp.phase_idx:
                if nlp.control_type == ControlType.CONSTANT or nlp.control_type == ControlType.NONE:

Suggestion:

`nlp.control_type in (ControlType.CONSTANT, ControlType.NONE)`

bioptim/optimization/solution.py line 1276 at r11 (raw file):

                        axis=1,
                    )
            elif nlp.control_type == ControlType.LINEAR_CONTINUOUS or nlp.control_type == ControlType.NONE:

remove this

Code quote:

 nlp.control_type == ControlType.LINEAR_CONTINUOUS or nlp.control_type == ControlType.NONE

@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Patch coverage: 94.54% and project coverage change: +0.01 🎉

Comparison is base (249e163) 81.54% compared to head (386be40) 81.56%.

❗ Current head 386be40 differs from pull request most recent head 07fc240. Consider uploading reports for the commit 07fc240 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #628      +/-   ##
==========================================
+ Coverage   81.54%   81.56%   +0.01%     
==========================================
  Files         110      110              
  Lines       12341    12359      +18     
==========================================
+ Hits        10064    10080      +16     
- Misses       2277     2279       +2     
Impacted Files Coverage Δ
bioptim/gui/check_conditioning.py 85.53% <0.00%> (ø)
bioptim/limits/penalty.py 91.11% <66.66%> (-0.31%) ⬇️
bioptim/optimization/optimization_variable.py 87.63% <92.85%> (+0.13%) ⬆️
bioptim/dynamics/integrator.py 95.67% <100.00%> (+0.05%) ⬆️
bioptim/dynamics/ode_solver.py 89.07% <100.00%> (-0.19%) ⬇️
bioptim/interfaces/solve_ivp_interface.py 96.80% <100.00%> (+0.03%) ⬆️
bioptim/optimization/non_linear_program.py 90.57% <100.00%> (+0.20%) ⬆️
bioptim/optimization/optimization_vector.py 95.73% <100.00%> (+0.01%) ⬆️
bioptim/optimization/solution.py 83.65% <100.00%> (ø)

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
Collaborator Author

@Kev1CO Kev1CO 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: 9 of 19 files reviewed, 12 unresolved discussions (waiting on @pariterre)


bioptim/dynamics/ode_solver.py line 155 at r5 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

len(nlp.external_forces)
up

done.

Copy link
Collaborator

@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.

Reviewed 2 of 4 files at r12, 1 of 2 files at r13, 1 of 5 files at r14, 6 of 6 files at r15, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @Kev1CO and @pariterre)


bioptim/dynamics/ode_solver.py line 135 at r11 (raw file):

Previously, Kev1CO wrote…

Done.

nlp.controls.cx if nlp.control_type in (ControlType.CONSTANT, ...) else ...


bioptim/dynamics/ode_solver.py line 135 at r15 (raw file):

            if nlp.control_type == ControlType.NONE
            else nlp.controls.cx
            if nlp.control_type == ControlType.CONSTANT

nlp.controls.cx if nlp.control_type in (ControlType.CONSTANT, ...) else ...


bioptim/optimization/optimization_variable.py line 602 at r15 (raw file):

    def _set_casadi_symbolic_callable(self, casadi_symbolic_callable: Callable = None):
        if casadi_symbolic_callable is None:
            raise ValueError()

if casadi_symbolic_callable is None and not isinstance(..., Callable):

"This entry should be either casadi.MX or casadi.SX"


bioptim/optimization/optimization_vector.py line 312 at r15 (raw file):

        p_idx = 0

        if self.ocp.nlp[0].control_type != ControlType.NONE:

self.ocp.nlp[0].control_type in (CONSTANT and LINEAR_CONTINUOUS...)

Code quote:

self.ocp.nlp[0].control_type

Copy link
Collaborator Author

@Kev1CO Kev1CO 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: 19 of 20 files reviewed, 15 unresolved discussions (waiting on @Ipuch and @pariterre)


bioptim/optimization/optimization_variable.py line 602 at r15 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

if casadi_symbolic_callable is None and not isinstance(..., Callable):

"This entry should be either casadi.MX or casadi.SX"

Done.


bioptim/optimization/optimization_vector.py line 312 at r15 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

self.ocp.nlp[0].control_type in (CONSTANT and LINEAR_CONTINUOUS...)

Done.

@Kev1CO Kev1CO changed the title [WIP] Sample data control [RTR] Sample data control Mar 18, 2023
Copy link
Collaborator Author

@Kev1CO Kev1CO 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: 16 of 20 files reviewed, 15 unresolved discussions (waiting on @Ipuch and @pariterre)


bioptim/dynamics/ode_solver.py line 135 at r11 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

nlp.controls.cx if nlp.control_type in (ControlType.CONSTANT, ...) else ...

Done.


bioptim/dynamics/ode_solver.py line 135 at r15 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

nlp.controls.cx if nlp.control_type in (ControlType.CONSTANT, ...) else ...

Done.


bioptim/limits/penalty.py line 670 at r11 (raw file):

Previously, pariterre (Pariterre) wrote…

This should be a CX...

Done.

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 3 of 4 files at r17, 7 of 7 files at r18, 1 of 1 files at r19, 2 of 2 files at r20, 1 of 1 files at r21, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Ipuch and @Kev1CO)


bioptim/optimization/optimization_variable.py line 3 at r20 (raw file):

import numpy as np
from casadi import MX, SX, vertcat
from typing import Callable

Typing is a built-in package. It should therefore appear first followed by a white space

Code quote:

from typing import Callable

bioptim/optimization/optimization_variable.py line 324 at r20 (raw file):

    mx_reduced: MX
        The reduced MX to the size of _cx
    _casadi_symbolic_callable: Callable

change the name to cx_constructor

Code quote:

_casadi_symbolic_callable

bioptim/optimization/optimization_variable.py line 578 at r20 (raw file):

class OptimizationVariableContainer:
    def __init__(self, variable_scaled=None, variables_unscaled=None, casadi_symbolic_callable: Callable = None):

Update this name

Code quote:

casadi_symbolic_callable

Ipuch
Ipuch previously approved these changes Mar 23, 2023
Copy link
Collaborator

@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.

Reviewed 2 of 4 files at r17, 7 of 7 files at r18, 1 of 1 files at r19, 2 of 2 files at r20, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Kev1CO and @pariterre)

@pariterre pariterre changed the title [RTR] Sample data control Sample data control Mar 23, 2023
Copy link
Collaborator Author

@Kev1CO Kev1CO 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 20 files reviewed, 7 unresolved discussions (waiting on @Ipuch and @pariterre)


bioptim/optimization/optimization_variable.py line 3 at r20 (raw file):

Previously, pariterre (Pariterre) wrote…

Typing is a built-in package. It should therefore appear first followed by a white space

Done.


bioptim/optimization/optimization_variable.py line 324 at r20 (raw file):

Previously, pariterre (Pariterre) wrote…

change the name to cx_constructor

Done.


bioptim/optimization/optimization_variable.py line 578 at r20 (raw file):

Previously, pariterre (Pariterre) wrote…

Update this name

Done.

@Kev1CO
Copy link
Collaborator Author

Kev1CO commented Mar 24, 2023

@pariterre RTM

@Kev1CO Kev1CO changed the title Sample data control [RTM] Sample data control Mar 24, 2023
@Kev1CO Kev1CO marked this pull request as ready for review March 24, 2023 16:23
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 2 of 3 files at r22, 1 of 1 files at r23, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Kev1CO)

@pariterre pariterre merged commit aca4749 into pyomeca:master Mar 30, 2023
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