strands-project/strands_qsr_lib

[QSRs] Also allow passing of `qsrs_for` in the `dynamic_args` within QSR namespace

Closed this issue · 8 comments

Allow to pass qsrs_for in the QSR namespace of dynamic_args. Still allow qsrs_for to be passed in kwargs but dynamic_args namespace takes precedence.

Waits for #107
Required as part of #112
This issue blocks #111

Following branch resolves this.

@cdondrup Have a look on the QTCs as I removed the raise thing.

Blocked till #107

That's fine, I believe I copied that from somewhere any way.

Just an additional comment, It would be nice that when the qsrs_for is given without a namespace it counts for every QSR requested. This can then be overridden for specific QSRs when using the namespace. Example:

{"qsrs_for": [("o1","o2")("o3","o4")], "qtcs": {"qsrs_for":("o2","o3")}}

this would result in all QSRs being created for ("o1","o2")("o3","o4") but all QTCs would be created for ("o2","o3"). This would remove a lot of overhead if you want to request several QSRs for the same object combination.

I don't see how this blocks #111. A simple rebase after #128 has been merged should fix this. Also, since we are changing to a non backward compatible version I think we should abolish the old way of setting qsrs_for and while we're at it, the name qsrs_for is not very self-explanatory. I don't really have a much better idea, maybe something like object_pairs, but I personally couldn't tell what this variable is good for just from its name.

How about one of these:

  • qsr_filter ?
  • qsr_select ?
  • qsr_only ?

Just an additional comment, It would be nice that when the qsrs_for is given without a namespace it counts for every QSR requested.

It already does so, priority given to the namespace.

To your above comments. qsrs_for' if short forgenerate_qsrs_for_the_followingoverwriting default behaviour of each QSR which typically will generate QSRs for all possible objects in aWorld_Trace`. E.g. you might have a World_Trace for full body skeleton and objects but require QSRs only for hands and objects... This allows you to do so easily.

I find it confusing to have both, a variable in the request message and the use inside the namespace via dynamic_args. This is not really readable or logical and I would strongly advise against this. I see two possibilities of solving this and be consistent in usage, i.e. not setting the same variable at two different places:

  1. The qsrs_for field takes a dictionary instead of a list of tuples where you can define namespaces or you don't:

    QSRlib_Request_Message(
    ...
    qsrs_for={"object_pairs": [("o1","o2"),("o3","o4"),("o1","o4")],
        "rcc8": {"object_pairs": [("o1","o3")]}
    }
    ...
    )

    where the namespaced one overrides the general one for the specified QSR.

  2. The qsrs_for field is removed and all of it is handled via dynamic_args:

    QSRlib_Request_Message(
    ...
    dynamic_args={"qsrs_for": [("o1","o2"),("o3","o4"),("o1","o4")],
        "rcc8": {"qsrs_for": [("o1","o3")]}
    }
    ...
    )

They both have benefits but also downsides. The benefits of the dynamic_args version are that you can just create one big dictionary with a consistent namespace for everything and pass it to the request message. The downside is, the dictionary becomes too complex and users and developers have to know how to parse it properly and what it can contain in the first place.

The benefits of the qsrs_for variable being a dictionary is that it is very much purpose bound and does not hide in the dynamic_args and is therefore easier to parse and understand. The downside, apart form some overhead regarding the namespaces, is the name qsrs_for itself as it is not very descriptive. I know now that it is short for generate_qsrs_for_the_following but that does not help you when you start working with the library. This is just a cryptic name that doesn't help you understand what it does.

Long story short, I believe that the solution implemented in this PR is the worst possible way of handling this because it sets the same variable at two different locations in the arguments. This is horrible to maintain and use! I don't favour of any of my above solutions so either (or maybe a third one I didn't think of yet) would be good for me but I strongly argue against doing it the way it is currently proposed in #131.