-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: test_entry_size; bound_checking biorbd_model #832
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Infa60)
bioptim/models/biorbd/biorbd_model.py
line 107 at r1 (raw file):
def check_q_size(self, q): if q.shape[0] > self.nb_q: raise ValueError(f"Length of q is too big. Expected size: {self.nb_q}, but got: {q.shape[0]}")
is too large?
Is not the right size? (what happens if too small... it does not seem to raise)
bioptim/models/biorbd/biorbd_model.py
line 634 at r1 (raw file):
@staticmethod def animate( solution: Any, show_now: bool = True, tracked_markers: list[np.ndarray, ...] = None, **kwargs: Any
If you cannot import "SolutionData" then do not write anything. It is not "Any" type
bioptim/models/biorbd/biorbd_model.py
line 643 at r1 (raw file):
check_version(bioviz, "2.0.0", "2.4.0") states = solution.states
This will not work... as states does not return "q" by itself
tests/shard1/test_entry_size.py
line 1 at r1 (raw file):
import pytest
Please rename the file for test_biorbd_model_size.py
tests/shard1/test_entry_size.py
line 5 at r1 (raw file):
from casadi import MX BIORBD_MODEL_PATH = "/home/lim/Documents/Bioptim/bioptim/bioptim/examples/muscle_driven_with_contact/models/2segments_4dof_2contacts_1muscle.bioMod"
This does not make sense
tests/shard1/test_entry_size.py
line 193 at r1 (raw file):
# # q and qdot valid but tau_activations not valid # with pytest.raises(ValueError, match="Length of tau is too big"): # model.torque(tau_activations_too_large, q_valid, qdot_valid)
Why is this commented?
tests/shard1/test_entry_size.py
line 315 at r1 (raw file):
# # muscle not valid # with pytest.raises(ValueError, match="Length of muscle is too big"): # model.muscle_activation_dot(muscle_too_large)
Why is this commented?
tests/shard1/test_entry_size.py
line 358 at r1 (raw file):
# # q and qdot valid but qddot not valid # with pytest.raises(ValueError, match="Length of qddot is too big"): # model.muscle_joint_torque(muscle_too_large, q_valid, qdot_valid)
Why is this commented?
tests/shard1/test_entry_size.py
line 427 at r1 (raw file):
# # q valid but qdot not valid # with pytest.raises(ValueError, match="Length of qdot is too big"): # model.tau_max(q_valid, qdot_too_large)
Why is this commented?
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #832 +/- ##
==========================================
+ Coverage 78.12% 78.22% +0.10%
==========================================
Files 140 140
Lines 16100 16189 +89
==========================================
+ Hits 12578 12664 +86
- Misses 3522 3525 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@pariterre there is a problem with black could we merge anyway? Worst case scenario, my next PR will resolve the black issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Infa60)
bioptim/models/biorbd/biorbd_model.py
line 107 at r1 (raw file):
Previously, pariterre (Pariterre) wrote…
is too large?
Is not the right size? (what happens if too small... it does not seem to raise)
change > for ==
bioptim/models/biorbd/biorbd_model.py
line 151 at r3 (raw file):
if muscle_size > self.nb_muscles: raise ValueError(f"Length of muscle is too big. Expected size: {self.nb_muscles}, but got: {muscle_size}")
The ">" should be a "=="
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Infa60)
@mickaelbegon @Infa60 @EveCharbie |
It was not purposely changed by Infa60, his modifications are due to the merging problem we talked about. |
All Submissions:
New Feature Submissions:
black . -l120 --exclude "external/*"
)?Changes to Core Features:
This change is