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

Adding potential new feature to pyexotica #519

Merged
merged 4 commits into from
Apr 18, 2019
Merged

Adding potential new feature to pyexotica #519

merged 4 commits into from
Apr 18, 2019

Conversation

cmower
Copy link
Collaborator

@cmower cmower commented Feb 21, 2019

I found it frustrating having to keep stop/starting ros launch in order to tune the rho parameters for task maps. To combat this issue I implemented a script interactive_rho_tuning.py that allows you to tune the rho's without having to constantly stop/start. See here for an example of its use.

If this sort of feature is something you would like in exotica let me know any changes/additions, otherwise feel free to close the pull request.

Checklist

  • code formatting: run find -name '*.cpp' -o -name '*.h' -o -name '*.hpp' | xargs clang-format-3.9 -style=file -i in the root directory

  • add unit test(s) (NA)

  • ensure tests build and check results: run catkin run_tests exotica && catkin_test_results (NA)

@cmower cmower changed the title [exotica_python, exotica_example] adding potential new feature Adding potential new feature to pyexotica Feb 21, 2019
@@ -0,0 +1,33 @@
import Tkinter
Copy link
Collaborator

Choose a reason for hiding this comment

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

New run dependency not included in the package.xml

@VladimirIvan
Copy link
Collaborator

dynamic_reconfigure was designed precisely for this purpose. Why use custom UI?

@cmower
Copy link
Collaborator Author

cmower commented Feb 25, 2019

@VladimirIvan exotica doesn't broadcast Rho values as configurable ros parameters so cannot be picked up by dynamic_reconfigure.

@wxmerkt
Copy link
Collaborator

wxmerkt commented Feb 25, 2019

I like the idea but haven't been able to test it in particular. I see your issues with dynamic_reconfigure and like the simplicity of this interface.

You can get a callback in dynamic_reconfigure to extract the values and set them to whatever you want - that could help? [I've only used that in C++ before]

@cmower
Copy link
Collaborator Author

cmower commented Feb 26, 2019

You can get a callback in dynamic_reconfigure to extract the values and set them to whatever you want - that could help? [I've only used that in C++ before]

This sounds like a good idea to look into. They list in the ros wiki for dynamic_reconfigure that the Python API is stable so I guess should work.

Two questions:

  1. The feature could be generalized to re-configure other parameters on-the-fly (eg. solver params). Would this be worth the effort or should I stick with Rho parameters?

  2. Is there a way to discriminate task maps based on their use as either cost terms or hard constraints? What would be useful is to have a function is_cost_term() (for example). Having this function would allow me to perform the following

In XML:

<Cost>
  <Task Task="ACostTaskMap" Rho="1"/>
</Cost>

<Equality>
  <Task Task="AnEqualityTaskMap"/>
</Equality>

In Python:

task_maps = problem.get_task_maps()
cost_task_maps = []
for k in task_maps.keys():
  task_map = task_maps[k]
  if task_maps[k].is_cost_term(): 
    # True -> task map used in cost function
    cost_task_maps.append(task_map)

# Now cost_task_maps is a list of 1 element
# i.e. the task map with the name "ACostTaskMap"

@wxmerkt
Copy link
Collaborator

wxmerkt commented Feb 26, 2019

  1. The feature could be generalized to re-configure other parameters on-the-fly (eg. solver params). Would this be worth the effort or should I stick with Rho parameters?

Only implement what you need for your own work right now, it can always be extended later once required.

2. Is there a way to discriminate task maps based on their use as either cost terms or hard constraints? What would be useful is to have a function is_cost_term() (for example). Having this function would allow me to perform the following

The equality, inequality, cost private members (of type Task or TimeIndexedTask) are or should be exposed as read-only for most problem types. You can get the tasks included in them from there - or add a convenience method to Task/TimeIndexedTask.

@wxmerkt
Copy link
Collaborator

wxmerkt commented Mar 27, 2019

What is the status of this pull request? It seems like after adding the run dependency it could be good?

@cmower
Copy link
Collaborator Author

cmower commented Apr 17, 2019

This hasn't been a priority for me for a while so I didn't look at it till now.

  • I've added a new save feature. See a demo here.
  • Simply using a try-except solves the error when trying to handle equality/inequality constraints.
  • Added icon for tk window
  • Added Tkinter to package.xml

@cmower
Copy link
Collaborator Author

cmower commented Apr 17, 2019

Seems Travis CI build failed. I do not know why?

@cmower
Copy link
Collaborator Author

cmower commented Apr 17, 2019

Prepend python- to Tkinter in package.xml. Perhaps this solves Travis CI issue...

@@ -14,4 +14,5 @@
<depend>shape_msgs</depend>
<depend>python-matplotlib</depend>
<depend>python-pyassimp</depend>
<depend>python-Tkinter</depend>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved, also added python-rospkg as per here

[exotica] added exotica icon
[exotica_examples] added an example of rho tuning
@cmower
Copy link
Collaborator Author

cmower commented Apr 18, 2019

  • Travis CI passed all checks
  • Squashed commits into 1 and force pushed

Waiting for final Travis CI checks to be completed and ready to merge as far as i can tell.


__all__=['Tuning']

class Tuning(object):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please select a more descriptive name for this class, e.g., UnconstrainedEndPoseProblemInteractiveCostTuning or InteractiveCostTuning. As far as I can tell it now only supports costs, not constraints - and only for end-pose problems.

import Tkinter as tk
import rospkg as rp

__all__=['Tuning']
Copy link
Collaborator

Choose a reason for hiding this comment

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

PEP8 formatting (here and in the example file)

self.master.winfo_toplevel().title("Rho Tuning")

# Set icon
# note, rp.RosPack().get_path('exotica') throws error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, because the core package is now exotica_core and exotica is only a metapackage - which does not work with ROS pack. A metapackage is like a stack, i.e., you can use rosstack, cf. ros/rospack#26. So your solution is:

rp.RosStack().get_path('exotica')


# Set icon
# note, rp.RosPack().get_path('exotica') throws error
icon = rp.RosPack().get_path('exotica_core')[:-13] + '/exotica/doc/images/EXOTica_icon.png'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like this workaround. It has some assumptions on exotica_core being next to exotica, as in our source builds. This may not hold. I have posted a fix using RosStack above.

print("Setting Rho parameters:")
for k in self.cost_task_map_names:
r = float(self.entries[k].get())
self.problem.set_rho(k, r)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: this only works for end-pose problems.

…interactive_cost_tuning

[exotica_python] new features to interactive_cost_tuning
  * reset button: resets params to the values originally specified in .xml
  * math feature in entries, you can now write math in entries
@cmower
Copy link
Collaborator Author

cmower commented Apr 18, 2019

  • This will work for unconstrained/constrained problems however the tuning is only for cost terms. It doesn't make sense for equality constraints and will only make sense for inequality constraints if you want to switch the sign and so essentially switching between <= and =>.
  • New math and reset features added, see demo here.
  • All other points raised above have been addressed in latest commit.

Copy link
Collaborator

@wxmerkt wxmerkt left a comment

Choose a reason for hiding this comment

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

Thank you for the quick turnaround - one typo but otherwise good to go.

@wxmerkt wxmerkt merged commit 5a28aa7 into master Apr 18, 2019
@wxmerkt wxmerkt deleted the cem-cost-tuning branch April 18, 2019 18:10
@wxmerkt
Copy link
Collaborator

wxmerkt commented Apr 18, 2019

Thank you for adding this feature :-)

wxmerkt added a commit that referenced this pull request Jun 22, 2020
Adding potential new feature to pyexotica
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.

3 participants