Add warmstart implementation for warmstart from reference points #111

kzorina · 2024-12-11T09:47:53Z

No description provided.

for more information, see https://pre-commit.ci

ArthurH91

few comments on order of imports & the test class could have docstrings to explain what's tested in each test, but otherwise looks good

agimus_controller/agimus_controller/warm_start_base.py

agimus_controller/agimus_controller/warm_start_reference.py

MaximilienNaveau

There are some issues with the WarmStartBase API.
Take a look at how the MPC is actually using it.
This is the proper API. I am sorry I have not seen it in the previous PR.

agimus_controller/agimus_controller/warm_start_base.py

agimus_controller/agimus_controller/warm_start_reference.py

agimus_controller/tests/test_warm_start.py

Co-authored-by: Arthur Haffemayer <[email protected]>

for more information, see https://pre-commit.ci

agimus_controller/agimus_controller/trajectory.py

agimus_controller/agimus_controller/warm_start_reference.py

agimus_controller/tests/test_warm_start_reference.py

agimus_controller/agimus_controller/warm_start_reference.py

numpy types Co-authored-by: Krzysztof Wojciechowski <[email protected]>

for more information, see https://pre-commit.ci

Kotochleb

When I think of it now, maybe the documentation should be filled in on the WarmStartBase class side and then it can be assumed to be inherited later on.

It should also be extended in the base class and WarmStartReference is still missing a loat of documentation

agimus_controller/agimus_controller/trajectory.py

agimus_controller/agimus_controller/warm_start_reference.py

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

for more information, see https://pre-commit.ci

agimus_controller/agimus_controller/warm_start_reference.py

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

for more information, see https://pre-commit.ci

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

for more information, see https://pre-commit.ci

Kotochleb

Just a few cosmetic changes. Otherwise, it's ok. I have weird feeling about those asserts, but I will be a hypocrite to tell you to do this and later change my mind

agimus_controller/agimus_controller/warm_start_base.py

agimus_controller/agimus_controller/warm_start_reference.py

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

for more information, see https://pre-commit.ci

agimus_controller/agimus_controller/warm_start_base.py

agimus_controller/agimus_controller/warm_start_reference.py

Kotochleb · 2025-01-02T11:52:21Z

I forgot to mention. Please take a look at this. This is an official documentation for google doc string formatting for the sphinx plugin. This is the formatting we want to expect to create sphinx documentation out of it

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

for more information, see https://pre-commit.ci

Kotochleb

Encoding in the files and module docstrings still needs to be discussed

Kotochleb · 2025-01-07T13:58:17Z

agimus_controller/agimus_controller/warm_start_base.py

+        ...
+
+    @abstractmethod
+    def setup(self, *args, **kwargs) -> None:


I think no args and kwargs are needed in here.

Shouldn't it allow for some arguments in the child classes? Ithouth those, I would assume it's an argumentless method

agimus_controller/agimus_controller/warm_start_base.py

agimus_controller/agimus_controller/warm_start_reference.py

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

MedericFourmy

tests are passing (after adding the unittest.main() call)

agimus_controller/tests/test_warm_start_base.py

agimus_controller/tests/test_warm_start_reference.py

for more information, see https://pre-commit.ci

MedericFourmy · 2025-01-07T15:15:36Z

agimus_controller/agimus_controller/warm_start_reference.py

+        list[npt.NDArray[np.float64]],
+    ]:
+        """
+        Generate initial values for a warm-start of the optimization problem.


I think we could follow the docstring format of warm_start_base.py here too for consistancy.

Also, I would explain the fact that the returned control trajectory init_us at the start of the description, after the sentence "Generate initial values..." (the fact that it is computed from RNEA and not taken from the initial_state.

kzorina and others added 5 commits December 11, 2024 09:44

add typing to warmstartBase

d6a3a14

add warmstart from reference

4be5ded

init traj point to have None values

aa16841

create tests for warmstart from reference

13536bd

[pre-commit.ci] auto fixes from pre-commit.com hooks

8d00f8e

for more information, see https://pre-commit.ci

ArthurH91 requested review from Kotochleb and ArthurH91 December 11, 2024 09:58

MaximilienNaveau self-requested a review December 11, 2024 10:08

ArthurH91 reviewed Dec 11, 2024

View reviewed changes

agimus_controller/agimus_controller/warm_start_base.py Outdated Show resolved Hide resolved

agimus_controller/agimus_controller/warm_start_reference.py Outdated Show resolved Hide resolved

agimus_controller/agimus_controller/warm_start_reference.py Outdated Show resolved Hide resolved

MaximilienNaveau requested changes Dec 11, 2024

View reviewed changes

kzorina and others added 7 commits December 11, 2024 11:30

Update agimus_controller/agimus_controller/warm_start_reference.py

abf790c

Co-authored-by: Arthur Haffemayer <[email protected]>

Update agimus_controller/agimus_controller/warm_start_reference.py

0bafb70

Co-authored-by: Arthur Haffemayer <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

e9a6f4d

for more information, see https://pre-commit.ci

divie warmstart tests into two files

9a1be8f

update imports to follow PEP 8

a51d9bf

add numpy typing and x0

a590c7f

[pre-commit.ci] auto fixes from pre-commit.com hooks

42f6b37

for more information, see https://pre-commit.ci

Kotochleb suggested changes Dec 11, 2024

View reviewed changes

kzorina and others added 7 commits December 11, 2024 15:39

Update agimus_controller/agimus_controller/trajectory.py

ec61b1f

numpy types Co-authored-by: Krzysztof Wojciechowski <[email protected]>

Update agimus_controller/agimus_controller/trajectory.py

ad2cfc2

numpy types Co-authored-by: Krzysztof Wojciechowski <[email protected]>

update typing

27d4ed1

change to finding torques with rnea

63853bb

[pre-commit.ci] auto fixes from pre-commit.com hooks

3b221ad

for more information, see https://pre-commit.ci

improve docstring

4f412ea

[pre-commit.ci] auto fixes from pre-commit.com hooks

bdb1c21

for more information, see https://pre-commit.ci

Kotochleb suggested changes Dec 11, 2024

View reviewed changes

kzorina and others added 3 commits December 11, 2024 16:36

Update agimus_controller/agimus_controller/trajectory.py

31a9032

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

Update documentation

c6a3e16

[pre-commit.ci] auto fixes from pre-commit.com hooks

0b65d7b

for more information, see https://pre-commit.ci

MaximilienNaveau requested changes Dec 11, 2024

View reviewed changes

agimus_controller/agimus_controller/warm_start_reference.py Outdated Show resolved Hide resolved

agimus_controller/agimus_controller/warm_start_reference.py Outdated Show resolved Hide resolved

kzorina and others added 6 commits December 12, 2024 16:25

Update agimus_controller/agimus_controller/trajectory.py

6c2e874

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

fb240f8

for more information, see https://pre-commit.ci

remove casting from tets

6c8c537

Update agimus_controller/agimus_controller/warm_start_reference.py

fb10156

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

add/extend docstrings

f4e88ec

[pre-commit.ci] auto fixes from pre-commit.com hooks

06032a4

for more information, see https://pre-commit.ci

kzorina requested a review from Kotochleb December 13, 2024 12:46

Kotochleb suggested changes Dec 13, 2024

View reviewed changes

agimus_controller/agimus_controller/warm_start_base.py Outdated Show resolved Hide resolved

agimus_controller/agimus_controller/warm_start_base.py Outdated Show resolved Hide resolved

agimus_controller/agimus_controller/warm_start_reference.py Show resolved Hide resolved

kzorina and others added 4 commits December 22, 2024 17:27

Update agimus_controller/agimus_controller/warm_start_base.py

dd328d4

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

Update agimus_controller/agimus_controller/warm_start_base.py

6d6277d

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

add docstring to warmstart

23f0da7

[pre-commit.ci] auto fixes from pre-commit.com hooks

984f988

for more information, see https://pre-commit.ci

kzorina requested a review from Kotochleb December 22, 2024 16:51

Kotochleb suggested changes Jan 2, 2025

View reviewed changes

kzorina and others added 5 commits January 6, 2025 15:47

Update agimus_controller/agimus_controller/warm_start_base.py

81c50b4

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

add format changes according to PR comments

f71f418

[pre-commit.ci] auto fixes from pre-commit.com hooks

5eb79de

for more information, see https://pre-commit.ci

add abstract setup class

48a8ae1

[pre-commit.ci] auto fixes from pre-commit.com hooks

155cee9

for more information, see https://pre-commit.ci

kzorina requested a review from Kotochleb January 7, 2025 12:59

Kotochleb suggested changes Jan 7, 2025

View reviewed changes

kzorina and others added 2 commits January 7, 2025 15:42

Update agimus_controller/agimus_controller/warm_start_base.py

4358c99

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

Update agimus_controller/agimus_controller/warm_start_reference.py

41308f3

Co-authored-by: Krzysztof Wojciechowski <[email protected]>

MedericFourmy requested changes Jan 7, 2025

View reviewed changes

agimus_controller/tests/test_warm_start_base.py Show resolved Hide resolved

agimus_controller/tests/test_warm_start_reference.py Show resolved Hide resolved

kzorina and others added 3 commits January 7, 2025 14:57

add unittest.main

9fc31c7

remove encoding and useless imports

28f2585

[pre-commit.ci] auto fixes from pre-commit.com hooks

6df285f

for more information, see https://pre-commit.ci

kzorina requested a review from MedericFourmy January 7, 2025 15:04

MedericFourmy reviewed Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warmstart implementation for warmstart from reference points #111

Add warmstart implementation for warmstart from reference points #111

kzorina commented Dec 11, 2024

ArthurH91 left a comment

MaximilienNaveau left a comment

Kotochleb left a comment

Kotochleb left a comment

Kotochleb commented Jan 2, 2025

Kotochleb left a comment

Kotochleb Jan 7, 2025

kzorina Jan 7, 2025

MedericFourmy left a comment

MedericFourmy Jan 7, 2025

Add warmstart implementation for warmstart from reference points #111

Are you sure you want to change the base?

Add warmstart implementation for warmstart from reference points #111

Conversation

kzorina commented Dec 11, 2024

ArthurH91 left a comment

Choose a reason for hiding this comment

MaximilienNaveau left a comment

Choose a reason for hiding this comment

Kotochleb left a comment

Choose a reason for hiding this comment

Kotochleb left a comment

Choose a reason for hiding this comment

Kotochleb commented Jan 2, 2025

Kotochleb left a comment

Choose a reason for hiding this comment

Kotochleb Jan 7, 2025

Choose a reason for hiding this comment

kzorina Jan 7, 2025

Choose a reason for hiding this comment

MedericFourmy left a comment

Choose a reason for hiding this comment

MedericFourmy Jan 7, 2025

Choose a reason for hiding this comment