Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add parameters to Models allowing robot joints to be parameterized #41

Merged
merged 159 commits into from
Feb 19, 2023

Conversation

joaomoura24
Copy link
Collaborator

Changes for adding option of optimizing a selection of the joints by specifying the joints that are parameterized when getting the robot model.

I tested all the changes for Inverse Kinematics, Inverse Differential Kinematics, and configuration planning.
I copied the figure_eight_plan.py file and made the necessary changes to keep one of the joints fixed, so the optimizer only optimizes 6 joints, here: link

The naming of functions might not be ideal, so please provide changes to the naming or the way it is done, as long as it doesn't break the goal functionality.

But also bare in mind that this functionality will not be used a lot by all the users, so it's ok to be a bit more cumbersome.
The added changes do not break the normal working of optas for the typical case that you optimize all of the joints.

@joaomoura24 joaomoura24 requested a review from cmower December 9, 2022 12:39
@joaomoura24
Copy link
Collaborator Author

Actually, lets hold the merge until beginning of January, cause just before leaving I found a bug which doesn't seem to come from my latest changes (it looks like it's from a new model I am using), but still I want to check.

@joaomoura24
Copy link
Collaborator Author

joaomoura24 commented Feb 12, 2023

@cmower, I rebased twice this branch.
It is now working, though i see that you made lots of changes in the functions naming.
Please, lets finalize the merge of this changes before making other changes to the master branch, or discuss a better way to achieve the same functionality.
I can meet you tomorrow to discuss this if needed.

Copy link
Owner

@cmower cmower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is moving in the right direction. However, I am not keen how the user is required to piece together the model optimized variables and parameters - the information on the ordering in the model array is all contained within the RobotModel class so this merging should be automatic.

This functionality should be abstracted to the Model class so TaskModel can take advantage of it too. I mention this in the comments that this we can leave as functionality only implemented, for now, in the RobotModel class. Please could you create an issue so we can implement this abstraction later.

Please see comments for specific changes needed.

Copy link
Owner

@cmower cmower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just some fairly minor points. I was thinking, could you double check the helper methods in the OptimizationBuilder class. I think some of these methods might break given this feature. E.g. enforce_model_limits may need to have the split/extract method called inside to make sure constraints are only enforced on optimized joints.

Copy link
Owner

@cmower cmower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more changes please.

@joaomoura24
Copy link
Collaborator Author

Don't merge this yet.
I am fixing the examples to run with the new 'q/x'.

@joaomoura24
Copy link
Collaborator Author

I updated the examples and ran them.
Note that i got errors in the examples to do with the issues #77 and #78.
I ran those two examples in the master branch as well just to check if the problem was still there.

@cmower cmower mentioned this pull request Feb 17, 2023
@joaomoura24
Copy link
Collaborator Author

I think this is ready to merge.
It still has the Changes requested on for some reason.
Could you take a look at it and see if you can merge?

@cmower cmower merged commit ca0eea7 into cmower:master Feb 19, 2023
@cmower
Copy link
Owner

cmower commented Feb 19, 2023

Thanks @joaomoura24!

@joaomoura24 joaomoura24 deleted the add-sel_opt_joints branch February 28, 2023 10:35
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants