-
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
[RTR] more detailed detailed costs #545
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: complete! all files reviewed, all discussions resolved (waiting on @Ipuch)
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 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Ipuch)
bioptim/optimization/solution.py
line 1392 at r2 (raw file):
] if print_only_weighted: print(f"{penalty.type.__str__()}: {val_weighted}")
is __str__()
actually needed? If so, call it as such str(penalty.type)
Code quote:
penalty.type.__str__()
bioptim/optimization/solution.py
line 1394 at r2 (raw file):
print(f"{penalty.type.__str__()}: {val_weighted}") else: print(f"{penalty.type.__str__()}: {val_weighted} (non weighted {val: .2f})")
2 decimal places seem short? Don't we expect the value to be down up to 10^-6 or something?
Code quote:
.2f
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, 2 unresolved discussions (waiting on @pariterre)
bioptim/optimization/solution.py
line 1392 at r2 (raw file):
Previously, pariterre (Pariterre) wrote…
is
__str__()
actually needed? If so, call it as suchstr(penalty.type)
It is needed yes, I find it more readable this way ...
It allows to print something like "Lagrange.MINIMIZE_CONTROL" or "Mayer.MINIMIZE_CONTROL"
bioptim/optimization/solution.py
line 1394 at r2 (raw file):
Previously, pariterre (Pariterre) wrote…
2 decimal places seem short? Don't we expect the value to be down up to 10^-6 or something?
Yep but we usually don't care about the non-weighted value. This was originally printed like this.
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, 1 unresolved discussion (waiting on @Ipuch and @pariterre)
bioptim/optimization/solution.py
line 1392 at r2 (raw file):
Previously, Ipuch (Pierre Puchaud) wrote…
It is needed yes, I find it more readable this way ...
It allows to print something like "Lagrange.MINIMIZE_CONTROL" or "Mayer.MINIMIZE_CONTROL"
f"{penalty.type}: {val_weighted}"
does not work??
@pariterre I think this is mergeable |
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: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @pariterre)
bioptim/optimization/solution.py
line 1392 at r2 (raw file):
Previously, pariterre (Pariterre) wrote…
f"{penalty.type}: {val_weighted}"
does not work??
indeed
detailed detailed costs
This change is