-
Notifications
You must be signed in to change notification settings - Fork 46
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
merge master into crazydynamics #464
Conversation
This reverts commit 508b883.
Codecov Report
@@ Coverage Diff @@
## CrazyDynamics #464 +/- ##
================================================
Coverage ? 80.96%
================================================
Files ? 85
Lines ? 9240
Branches ? 0
================================================
Hits ? 7481
Misses ? 1759
Partials ? 0 Continue to review full report at Codecov.
|
This is ready. |
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 4 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Ipuch)
bioptim/gui/graph.py
line 10 at r5 (raw file):
from ..optimization.parameters import Parameter from ..misc.enums import Node from bioptim import InterpolationType
Please refer using local and not installed (..SOMETHING)
bioptim/gui/graph.py
line 418 at r5 (raw file):
title = ["", "Bounds"] elif bounds.type == InterpolationType.LINEAR: title = ["", "Beginning", "End"]
Please use a "else" with a raise NotImplementedError to encapsulate all the non implemented cases
Code quote:
if bounds.type == InterpolationType.CONSTANT_WITH_FIRST_AND_LAST_DIFFERENT:
title = ["", "Beginning", "Middle", "End"]
elif bounds.type == InterpolationType.CONSTANT:
title = ["", "Bounds"]
elif bounds.type == InterpolationType.LINEAR:
title = ["", "Beginning", "End"]
tests/test_penalty.py
line 64 at r5 (raw file):
nu = nu * 2 if implicit else nu nu = nu + 3 if implicit and with_contact else nu
if implicit:
nu *=2
if with_contact:
nu += 3
Code quote:
nu = nu * 2 if implicit else nu
nu = nu + 3 if implicit and with_contact else nu
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pariterre)
bioptim/gui/graph.py
line 10 at r5 (raw file):
Previously, pariterre (Pariterre) wrote…
Please refer using local and not installed (..SOMETHING)
done
bioptim/gui/graph.py
line 418 at r5 (raw file):
Previously, pariterre (Pariterre) wrote…
Please use a "else" with a raise NotImplementedError to encapsulate all the non implemented cases
There is no error if not executed. So I placed a "else return"
tests/test_penalty.py
line 64 at r5 (raw file):
Previously, pariterre (Pariterre) wrote…
if implicit: nu *=2 if with_contact: nu += 3
done
…ith lagrangian objectives with trapezoidal integration.
small fix manage unit target
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 r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Ipuch)
bioptim/gui/graph.py
line 418 at r5 (raw file):
Previously, Ipuch (Pierre Puchaud) wrote…
There is no error if not executed. So I placed a "else return"
That is exactly the problem, it should create an error, as the other case (that we did not think of yet) may create a problem some day. The NotImplemented case should be raise and not just a return
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.
Reviewable status: 3 of 10 files reviewed, 1 unresolved discussion (waiting on @pariterre)
bioptim/gui/graph.py
line 418 at r5 (raw file):
Previously, pariterre (Pariterre) wrote…
That is exactly the problem, it should create an error, as the other case (that we did not think of yet) may create a problem some day. The NotImplemented case should be raise and not just a return
done
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 7 files at r7, 5 of 5 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Ipuch)
This change is