Implemented time in dynamic

[pariterre]

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 

[codecov]

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (4e90df7) 78.22% compared to head (c0e1952) 78.32%.
Report is 10 commits behind head on master.

Files	Patch %	Lines
bioptim/limits/constraints.py	14.28%	6 Missing ⚠️
bioptim/limits/penalty.py	25.00%	3 Missing ⚠️
bioptim/optimization/solution/solution.py	94.91%	3 Missing ⚠️
bioptim/limits/multinode_penalty.py	50.00%	1 Missing ⚠️
bioptim/optimization/optimization_variable.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #843      +/-   ##
==========================================
+ Coverage   78.22%   78.32%   +0.10%     
==========================================
  Files         140      140              
  Lines       16189    16232      +43     
==========================================
+ Hits        12664    12714      +50     
+ Misses       3525     3518       -7     

Flag	Coverage Δ
unittests	78.32% <90.14%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[EveCharbie]

Thanks a lot for this PR :)

Reviewed 25 of 30 files at r1, all commit messages.
Reviewable status: 25 of 30 files reviewed, 5 unresolved discussions (waiting on @pariterre)

---

bioptim/limits/penalty_controller.py line 135 at r1 (raw file):

        mx = vertcat(self.time.mx, dt_mx)
        dt_cx = self.dt.cx
        cx = vertcat(self.time.cx, dt_cx)

This would be easier for me to understand:

mx = vertcat(self.time.mx, self.dt.mx)
cx = vertcat(self.time.cx, self.dt.cx)

Code quote:

        dt_mx = self.dt.mx
        mx = vertcat(self.time.mx, dt_mx)
        dt_cx = self.dt.cx
        cx = vertcat(self.time.cx, dt_cx)

---

bioptim/limits/penalty_controller.py line 182 at r1 (raw file):

        tp = OptimizationVariableList(self._nlp.cx, self._nlp.phase_dynamics == PhaseDynamics.SHARED_DURING_THE_PHASE)
        n_val = self._nlp.time_mx.shape[0]

I thought time.mx was going to replace completely time_mx?

Code quote:

time_mx

---

bioptim/limits/penalty_controller.py line 198 at r1 (raw file):

        tp = OptimizationVariableList(self._nlp.cx, self._nlp.phase_dynamics == PhaseDynamics.SHARED_DURING_THE_PHASE)
        n_val = self._nlp.tf_mx.shape[0]

same?

Code quote:

tf_mx

---

tests/shard2/test_cost_function_integration.py line 262 at r1 (raw file):

                np.testing.assert_almost_equal(j_printed, 34.52084504124038)
            else:
                np.testing.assert_almost_equal(f[0, 0], 17.72098984554101)

Is that the bug that was fixed for trapezoidal integration of penalties ?

Code quote:

17.72098984554101

---

tests/shard4/test_solution.py line 36 at r1 (raw file):

        np.testing.assert_almost_equal(time[4], 0.8)
    else:
        np.testing.assert_almost_equal(time.shape, (51, 1))

Could you explain why the shape of the time vector has changed not by x2 ?

Code quote:

(51, 1)

[pariterre]

Reviewable status: 20 of 31 files reviewed, 9 unresolved discussions (waiting on @EveCharbie)

---

bioptim/limits/penalty_controller.py line 135 at r1 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

This would be easier for me to understand:

mx = vertcat(self.time.mx, self.dt.mx)
cx = vertcat(self.time.cx, self.dt.cx)

Indeed, done

---

bioptim/limits/penalty_controller.py line 182 at r1 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

I thought time.mx was going to replace completely time_mx?

nlp.time_mxis what is return by the controller. From the user standpoint, they should indeed not use time_mx but they should use time.mx (or .cx) but this value still need to be attached to the actual variable of the problem, which are still stored in nlp.
In brief, this means even though nlp.time_mx exists, here is the only place that it should be referenced

---

bioptim/limits/penalty_controller.py line 198 at r1 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

same?

Same answer, this is to contruct the tf OptimizationVariable. Another way could have been to directly store this OptimizationVariable in nlp.tf, which would be a good idea to standardize the use of time accross bioptim. This required a bit of extra work that I did not put in, and decided it was okay to simply create an interface on the spot each time tf is called. But I agree this would be cleaner to store tf (and other time related variables) in nlp in the first place! Maybe a "good first issue"?

---

bioptim/optimization/solution/solution.py line 10 at r1 (raw file):

from matplotlib import pyplot as plt

from .solution_data import SolutionData, SolutionMerge, TimeAlignment, TimeResolution

TimeResolution is not necessary anymore
Done

---

bioptim/optimization/solution/solution.py line 336 at r1 (raw file):

        to_merge: SolutionMerge | list[SolutionMerge] = None,
        continuous: bool = True,
    ) -> list | np.ndarray:

Remove this interface
Done

---

bioptim/optimization/solution/solution.py line 563 at r1 (raw file):

            raise NotImplementedError("NODE_SPAN is not implemented for states")
        else:
            raise ValueError("Unrecognized time_resolution")

Remove this interface
Done

---

bioptim/optimization/solution/solution.py line 612 at r1 (raw file):

        return data if len(data) > 1 else data[0]

    def controls(

Remove this interface

[pariterre]

Reviewable status: 20 of 31 files reviewed, 9 unresolved discussions (waiting on @EveCharbie)

---

tests/shard2/test_cost_function_integration.py line 262 at r1 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

Is that the bug that was fixed for trapezoidal integration of penalties ?

Yes, this value has changed from the old_master to the new_master. I double checked by computing the value using the LINEAR_CONTINUOUS as if it was CONSTANT and compared with CONSTANT and the new values correspond. Since I haven't done such testing previously, I trust more the new values

---

tests/shard4/test_solution.py line 36 at r1 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

Could you explain why the shape of the time vector has changed not by x2 ?

Because time is now fully alligned with its underlying structure. So I you align with DECISION_STATES for RK4, you expect only one value per node. Previously it was always returning t_span, that is initial time of the node and final time of the node. This does not correspond with the expected alignment of states. If time was retrieved using t_span then it would be 101.

[pariterre]

Reviewable status: 20 of 31 files reviewed, all discussions resolved (waiting on @EveCharbie)