Issue1062

Title Support case sensitivity in action names and objects
Priority feature Status resolved
Superseder Nosy List jendrik, malte, mkatz, silvan
Assigned To Keywords
Optional summary

Created on 2022-08-15.14:41:30 by mkatz, last changed by mkatz.

Files
File name Uploaded Type Edit Remove
case.patch mkatz, 2022-08-31.06:11:23 application/octet-stream
case.patch mkatz, 2022-08-31.21:53:04 application/octet-stream
Messages
msg10819 (view) Author: mkatz Date: 2022-08-31.21:53:04
Here is a new patch, with the changes as suggested
msg10818 (view) Author: mkatz Date: 2022-08-31.15:15:29
Thank you for the review!
- removed, my apologies, that was added by vscode :)
- I suppose I could process it once and then go over the parsed list and 
"duplicate" it, converting to lower case.
- I also think I did not handle goals correctly. The reason for handling goals was 
only to have the object names match. I will try to make it generic.
- I will drop axioms, that was too much eagerness on my side :) For init, the same 
as for goal, I only change object names so the grounded predicates would match to 
the names given in the (:objects ) section.
The only thing I want to achieve is the match in the case of the plans outputted, 
nothing else.
msg10817 (view) Author: malte Date: 2022-08-31.11:50:06
Some comments:

- Some cleanup could be done, e.g. you added a line "from re import T" that I think is unused? (I didn't see a line that uses "T".)

- If we were to merge this, processing the input file twice in this way would not work because it breaks a use case we currently support, namely the input coming from a file that is "read-once". This is useful for things like Unix pipes or sockets where you want to create the PDDL programmatically without putting it onto an actual physical file system (to save time and memory, to avoid complications regarding access rights, full hard disks etc. and to avoid having to clean up afterwards). If you use things like stdin or Unix FIFO pipes as the input file (which you can currently do using things like /dev/stdin), the change would break this. One use case of this kind that we've had in the past is for really large PDDL inputs (tens/hundreds of MB), which you can currently keep compressed on disk and uncompress them on the fly via pipes without creating an intermediate file (which would break our experiment environmene because of disk usage and file system bandwidth).

- I don't think the handling of the goal is general enough. Goals are in general arbitrary logical formulas, but it looks like you're assuming lists? For example, what about ADL goals with forall/exists/or etc. Also, for STRIPS, do you correctly handle the two cases "(atom)" (not a conjunction, single atom) and "(and (atom_1) ... (atom_n))"?

- Preserving case for axioms/init/goal does nothing in Fast Downward itself because these things are not printed anywhere except perhaps in some debug-style code. So it's hard to know if these work as designed in the code because I don't know what you want to do with these later in the code. I assume you have other changes in the code that print these? The main reason I'm asking is because of axioms. In PDDL axioms don't have a name, so I'm not sure what you want to preserve there, which affects what the intended behaviour should be. (In Fast Downward, axioms are represented like actions, so we assign something to the name field because actions need to have names, but I'd think of this as an internal representation decision.) I think in Fast Downward we just used the name of the predicate in the head, and I think your code extracts this from the axiom definition, not the predicate definitions. This means that capitalization might not be unique for the same thing; if you have predicate "FooBar" with two axioms that spell this as "foobar" and "FOOBAR", if you absolutely want a name for the axiom, it probably makes more sense to have "FooBar" as the name for all axioms that are about the same derived predicate rather than calling one axiom "foobar" and the other "FOOBAR", which I think is what the code currently does. (But I only looked at it very briefly.) But this also depends on what your intended behaviour actually is -- perhaps want you really want is to preserve every occurrence of every spelling the actual way it originally is, which is not the current design of the patch.

- Again, I don't know what the use case is in which you print the goal because this is not something that Fast Downward currently does, but I think it is inconsistent to preserve case for the parts of the goal description that are predicate and object names, but not for the rest of the goal specification. Currently, if I give my goal as "(AND (Or (X) (y)) (or (x) (Y)))", your code could potentially change the capitalization of (X) and (x) and of (y) and (Y), but I don't think it would have the information to preserve the capitalization of "AND" and "Or"/"or". I think if the argument is that it's a bug for the user to rewrite things from the way they wrote them, then that same argument can be made for "AND" etc.

- Related to this, if you take the argument "the user wants their capitalization preserved" seriously, perhaps you should actually preserve the capitalization that they use. I think what your code does is consider the first spelling of a predicate name/object in the files as canonical and replace every other spelling used elsewhere by that one. I think it would be more internally consistent to keep all atom occurrences the way they user specified them if the idea is to preserve the users' intentions.

- Again related to goals, you also would want to consider how to deal with variables in goals like "(forall (?MyThing - thing) (IsDone ?MyThing)). I don't remember off-hand what the code currently does, but I'm sure it doesn't keep "?MyThing" in its original capitalizatin. At minimum it changes it to "?mything", but I think there is also further processing that can lead to further renaming and then more complex things like replacing such goals by a new auxiliary axiom, so the goal would look very different from what is in the input. (Something like "(@new-axiom-0))".) The code also does other transformations and simplifications like dropping static goals etc., so the goal can look quite different from the original input for many reasons.
msg10816 (view) Author: malte Date: 2022-08-31.09:11:38
Thanks Michael, I'll have a look!
msg10815 (view) Author: mkatz Date: 2022-08-31.06:11:23
Understood. I would still ask to help with providing a patch. I have created one, 
which seems to work in my limited testing, but it is kind of ugly. Basically, I 
parse twice, once as before, once without changing the case. In the second parse, 
I only gather objects + constant (for their names) and action and axiom names. 
Then, I replace the lowercased names with these in the result of the first parse, 
replacing also the object names in init and goal.
It is probably not the most effective and definitely not the most elegant 
solution. Your comments and suggestions are more than welcome.
msg10813 (view) Author: malte Date: 2022-08-30.14:42:23
Another example of a similar language, also now quite out of fashion, is XML/HTML. XML ignores case for tags and attributes, and XML processing tools routinely normalize tags in their output. Not everybody is happy with this, and I don't think it's a good design choice for the language. But I think solving this at the level of Fast Downward fights against the conventions of the PDDL ecosystem, and it's better to do it at a wrapper level.
msg10812 (view) Author: malte Date: 2022-08-30.12:28:37
Ignoring capitalization is part of the spec, and normalizing capitalization is part of the heritage. It's not a bug but a clash of expectations. As a test, I just installed clisp on my Ubuntu machine and looked what its repl does:

(defun add (x y) (+ x y))
ADD

It changes my "add" to "ADD" because capitalization is ignored, and the normalized form in that system is all-uppercase. The point is not to annoy users, but to communicate clearly that capitalization is ignored, which is important because otherwise people will be suprised that "add" and "ADD" cannot be different things/functions in the language. (In contrast, in C++/Java it's very common to rely on case to have things of different identities, writing things like "Action action;".)

In the planning world, tt's not just Fast Downward. FF converts everything to upper-case. VAL converts everything to lower-case.

If I specify an action as "(go-left     ?x ?y)" and then in the output I see "(go-left foo bar)", is this a bug because I removed whitespace that the user presumably intentionally put in? Capitalization in PDDL is at the same syntactic level as whitespace or comments.

Basing syntax on LISP is not a modern design choice and not the one I would have used, but it is what PDDL is. I don't think implementing a workaround for this into Fast Downward for people that want something different is a good idea. It's the wrong place to solve the problem. What if they want to switch to a different planner tomorrow, and now their case preservation is gone again? I think it makes a lot more sense to solve this outside the planner core.

You mention that people use things like tarski, and that's exactly the point I wanted to make in the previous message. A library that wraps around the actual planning algorithm is exactly the place where I would solve this because this is a place where I have a semantic barrier, and I can make it work independently of the planner.

Regarding your last questions, it's the implementation effort only. I wouldn't expect a big performance hit even for large tasks because if I recall correctly we don't rely on the action/atom names anywhere for things like hashing, so wouldn't need to store two separate representation (preserved case and normalized case). An exception for this is the translator parsing stage where we check for things like duplicate action and predicate names; they would need to be adapted to consider both the normalized form (to check duplicates) and the case-preserved form (to keep it available). But this would be a comparatively minor hit with enough implementation care that throws away the duplicated information after parsing.

If you'd like to make this change as a patch, I think you'd only need to change the translator. The C++ part uses the action names from its input file in whatever form they are without any processing; for example, it would also preserve spaces. (I just tested this a little bit by manually changing the output.sas file.)
msg10811 (view) Author: mkatz Date: 2022-08-29.19:58:43
While I understand your point, let me claim that from the user perspective this 
can be a deal breaker. From their point of view, if they specify actions in one 
case, and the planner outputs the plans in a different case, it's a bug. Further, 
sometimes they barely see the PDDL and don't really know what it is, specifying 
the problems in, e.g., Tarski.
Could you please explain more about the complication to existing code? Is it just 
the effort in implementation or do you expect significant computational overhead?
msg10810 (view) Author: malte Date: 2022-08-29.16:32:01
Hi Michael, I'm against this. It complicates the code, and I don't see enough benefit. For better or worse, PDDL derives from LISP, and in LISP canonalizing letters (to either all-lowercase or all-uppercase) is the standard behaviour. It also has the benefit of teaching people that PDDL names are not case-sensitive.

(BTW, "case sensitivity" would be semantically wrong, contradicting the PDDL standard. I think what you're looking for here is called "case preservation", which is different from "case sensitivity". Case sensitivity would mean that you can use "a" and "A" as separate objects, which you can't in PDDL. For example, Windows filesystems are by default case-preserving, but not case-sensitive. Linux filesystems are by default case-sensitive.)

I see the point of your argument, but for me the cost/benefit of case preservation is not right, at least with our current code. I would suggest solving this at an interface layer outside the planner rather than inside the planner. This would currently be complicated because the interface layer would have to reimplement (some) PDDL parsing to be able to extract action/object names to be able to translate back the plans. In the future, when we have better programmatic interfaces to the planner, I hope this would be much easier to implement, and I'd be happy to revisit this suggestion at this point and look at a solution at that new layer.

Closing this because some time ago we started a policy only to keep feature requests open if there is a near-/medium-term plan to make them happen to avoid clutter. But we can of course continue the discussion, here or elsewhere.
msg10809 (view) Author: silvan Date: 2022-08-26.13:02:41
Moving Michael's original text to a change note entry (rather than the summary).

Currently, all strings in PDDL are converted to lower case and as a result, the 
plans outputted are in lower case and might not match the case specified in PDDL.
Users that are less familiar with planning and use planners for their applications 
expect the plans to match the action names they define. It would be great if we 
could support this functionality.
Suggested solution:
While string comparison can be done by comparing the lowercase versions, we can 
define the canonical names for all elements
1. Lifted action names are used only once in PDDL and therefore okay.
2. For objects, we can use the names from the (:objects) section of the problem 
file as canonical.
3. Predicates are optional, since they don't appear on plans, but if we want to 
match the case in PDDL, we can consider the case in (:predicates) as canonical.
History
Date User Action Args
2022-08-31 21:53:04mkatzsetfiles: + case.patch
messages: + msg10819
2022-08-31 15:15:29mkatzsetmessages: + msg10818
2022-08-31 11:50:06maltesetmessages: + msg10817
2022-08-31 09:11:38maltesetmessages: + msg10816
2022-08-31 06:11:23mkatzsetfiles: + case.patch
messages: + msg10815
2022-08-31 02:15:20mkatzsetmessages: - msg10814
2022-08-31 02:15:11mkatzsetfiles: - case.patch
2022-08-30 19:39:29mkatzsetfiles: + case.patch
messages: + msg10814
2022-08-30 14:42:23maltesetmessages: + msg10813
2022-08-30 12:28:37maltesetmessages: + msg10812
2022-08-29 19:58:43mkatzsetmessages: + msg10811
2022-08-29 16:32:01maltesetstatus: chatting -> resolved
messages: + msg10810
2022-08-26 13:02:41silvansetstatus: unread -> chatting
nosy: + malte, jendrik, silvan
messages: + msg10809
summary: Currently, all strings in PDDL are converted to lower case and as a result, the plans outputted are in lower case and might not match the case specified in PDDL. Users that are less familiar with planning and use planners for their applications expect the plans to match the action names they define. It would be great if we could support this functionality. Suggested solution: While string comparison can be done by comparing the lowercase versions, we can define the canonical names for all elements 1. Lifted action names are used only once in PDDL and therefore okay. 2. For objects, we can use the names from the (:objects) section of the problem file as canonical. 3. Predicates are optional, since they don't appear on plans, but if we want to match the case in PDDL, we can consider the case in (:predicates) as canonical. ->
2022-08-15 14:41:30mkatzcreate