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

Added cost details to sol #440

Merged
merged 11 commits into from
Mar 30, 2022
Merged

Conversation

EveCharbie
Copy link
Collaborator

@EveCharbie EveCharbie commented Feb 7, 2022

This change is Reviewable

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #440 (47a4293) into master (c3cc7d7) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##           master     #440   +/-   ##
=======================================
  Coverage   80.47%   80.48%           
=======================================
  Files          85       85           
  Lines        8985     8996   +11     
=======================================
+ Hits         7231     7240    +9     
- Misses       1754     1756    +2     
Impacted Files Coverage Δ
bioptim/examples/getting_started/pendulum.py 71.42% <0.00%> (-2.65%) ⬇️
bioptim/optimization/solution.py 84.80% <92.30%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3cc7d7...47a4293. Read the comment docs.

@pariterre
Copy link
Member

I don't understand the purpose of "to_console"? What happens if you put False, doesn't it just print nothing (which kind of doesn't make sense)?

@EveCharbie
Copy link
Collaborator Author

Yes, if to_console=False, nothing is print to the console but the values are added to sol. This might be great if you are multi-starting and want to log the console or if you want to be able to follow what is happening in the console with a print_level very low. However, I would understand if you prefer to remove this option.

@pariterre pariterre changed the title added cost details to sol [WIP] Added cost details to sol Mar 8, 2022
@pariterre
Copy link
Member

As verbally discussed:
Please create a separate function as it is not intended that printing modifies anything internally. If you think your function shares things with this one, it makes sense to create subfunctions that can be called by both of them!

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.

See previous comment

@EveCharbie EveCharbie changed the title [WIP] Added cost details to sol Added cost details to sol Mar 9, 2022
@EveCharbie
Copy link
Collaborator Author

I'm on black version :
black 22.1.0 pyhd8ed1ab_0 conda-forge
And no file would have been changed on my computer :/

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 3 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EveCharbie)


README.md, line 356 at r3 (raw file):

```python
sol.print_cost()  # For printing their values in the console
sol.detailed_cost_values_to_sol()  # For adding the objectives details to sol for later manipulations

sol.compute_detailed_cost_values() (to sol is redundant)


tests/test_global_getting_started.py, line 74 at r3 (raw file):

        # detailed cost values
        sol.detailed_cost_values_to_sol()
        np.testing.assert_almost_equal(sol.detailed_cost[0]['cost_value_weighted'], 41.57063948309302)

Maybe to help for future, instead of hard coding the value, you can compare it to f[0, 0] that is already valided? Like that if it ever change, we won't have to update the 2 lines for each test that changed?

@pariterre
Copy link
Member

I like what you have done so far, I think it is a great addition :)

@EveCharbie
Copy link
Collaborator Author

@pariterre are we good with this?

@pariterre
Copy link
Member

This commit 27eeac3
removed the folder "external" it is not possible anymore to use ACADOS. Please revert to the previous commit and recommit your last commit (I can help if you are affraid of losing work)

@EveCharbie
Copy link
Collaborator Author

I'm so sorry it happened again!
Is it ok now?

@pariterre
Copy link
Member

@EveCharbie
Unfortunately Submodules can't only be "revert", you have to checkout to the previous commit, then recommit the stuff you need to keep. Please note, that this process CAN lose work. I am availble if need to make sure you don't loose work ;)

pariterre
pariterre previously approved these changes Mar 25, 2022
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 3 of 7 files at r4, 3 of 7 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @EveCharbie)

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 1 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @EveCharbie)

@pariterre pariterre merged commit 0a86e9a into pyomeca:master Mar 30, 2022
@EveCharbie EveCharbie deleted the print_to_variables branch May 30, 2023 07:14
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

2 participants