search

stack-of-tasks / sot-core

Hierarchical task solver plug-in for dynamic-graph
BSD 2-Clause "Simplified" License
19 stars 32 forks source link

[Python] Expose Flags and signal of Flags #164

Closed jmirabel closed 4 years ago

jmirabel commented 4 years ago

I open this PR for discussion. The things to be discussed are the API of Flags and the necessary Python imports.

  1. With my new implementation, it is not possible to do feature.selec.value = "111" to set the signal value. This is the method I have always seen be used. Instead, one must do feature.selec.value = Flags("111").
  1. At the moment, the API of Flags is such that 
    f = Flags(0)
f.set("10")
for i in range(6):
print(f(i))

    will print

    False True

I find this very counter-intuitive.

  1. It would be better if the __init__.py could import the wrap.so. I know there has been an effort to make the init.py empty. How can I make sure wrap.so is imported with the module ?

Given 1. and 2., I see 3 options: a. fix 2. and don't fix 1. so that people will get an error where they should change the order of the flag. b. fix 1. and don't fix 2. so that code that used to work will still work. (requires some tricky changes). c. don't fix anything.

I would vote for a.

jmirabel commented 4 years ago

This needs https://github.com/stack-of-tasks/dynamic-graph-python/pull/64

Issue 3 can also be fixed by making sure all the entities that have a flag import wrap.so or call the function that expose Flags and its signals. Although a bit cumbersome, this is possible.

Assuming we fix 3 and, eventually, the Python code is update, this should fix our Talos integration issue.

jmirabel commented 4 years ago

@nim65s I shouldn't worry about CI, should I ?

nim65s commented 4 years ago

Thanks for taking some time fore this !

I also think that it is better to ensure "people will get an error where they should change the order of the flag.", so I would vote for a.

And I guess this should be relatively easy for me to just launch the integration tests with that, and and some Flags() at lines pointed by the stacktraces I'll get.

About 3, nothing is done yet, so you if you need to put stuff in __init__.py now, do it, we'll figure how to make relocatable packages later : this is a different issue, as this feature is not working yet.

And no, don't worry about the CI, it can't work in its current state: the integration tests will give us a go/no-go.

jmirabel commented 4 years ago

I implemented a. and a solution to 3. This can now be tested.