SoarGroup / Soar

Soar, a general cognitive architecture for systems that exhibit intelligent behavior.
http://soar.eecs.umich.edu
Other
328 stars 70 forks source link

Soar doesn't quote empty string attribute names in outputs #484

Open garfieldnate opened 1 month ago

garfieldnate commented 1 month ago

Source the following:

sp {foo
    (state <s> ^superstate nil)
-->
    (<s> ^|| hi)
}
step 1
p s1

This outputs the following:

(S1 ^ hi ^epmem E1 ^io I1 ^reward-link R1 ^smem L1 ^superstate nil ^type state)

We generally expect the output to be well-formed LHS syntax, but the bare caret will not parse if you try to use it in a production. This causes issues if you do any kind of processing of the output that then becomes input to Soar (e.g. constructing new productions from Soar output). It also wrongly suggests to the user that the bare caret syntax is legal.

A simple fix for this would be to quote empty string attribute names in the output of print.

@PLindes Doubts whether an empty attribute name should even be legal in Soar (deferring to @johnlaird for a decision on that).

PLatCIC commented 1 month ago

I'm the one that originally brought up this issue, and from my perspective this is not addressing the real problem.

The original problem was in working with Thor-Soar. The Python code that takes data from AI2-Thor and puts a list of objects on the input link has a problem with a fill-liquid1 property. For the Bowl4 object it puts this on the input link:

p -d 3 o5 (O5 ^object-handle Bowl4 ^property P38 ^property P37 ^property P36 ^property P35 ^property P34 ^property P33 ^property P32 ^property P31 ^property P30 ^property P29 ^property P28 ^property P27 ^property P39) (P38 ^property-handle mass ^type visual ^values V38) (V38 ^|0.47| 1.000000) (P37 ^property-handle fill-liquid1 ^type visual ^values V37) (V37 ^ 1.000000) (P36 ^property-handle contents1 ^type visual ^values V36) (V36 ^empty1 1.000000) (P35 ^property-handle is-dirty1 ^type visual ^values V35) (V35 ^not-dirty1 1.000000) (P34 ^property-handle is-broken1 ^type visual ^values V34) (V34 ^not-broken1 1.000000) (P33 ^property-handle is-grabbed1 ^type visual ^values V33) (V33 ^not-grabbed1 1.000000) (P32 ^property-handle affordance1 ^type visual ^values V32) (V32 ^dirtyable1 1.000000) (P31 ^property-handle affordance1 ^type visual ^values V31) (V31 ^grabbable1 1.000000) (P30 ^property-handle receptacle ^type visual ^values V30) (V30 ^receptacle 1.000000) (P29 ^property-handle temperature ^type visual ^values V29) (V29 ^room-temp1 1.000000) (P28 ^property-handle is-reachable1 ^type visual ^values V28) (V28 ^not-reachable1 1.000000) (P27 ^property-handle category ^type visual ^values V27) (V27 ^bowl 1.000000) (P39 ^property-handle material ^type visual ^values V39) (V39 ^ceramic1 1.000000)

Notice this part:

(P37 ^property-handle fill-liquid1 ^type visual ^values V37) (V37 ^ 1.000000)

The attribute of the wme on V37 has no attribute name. This seems to me that it may be invalid Soar, but even if it is not I think there should be a non-empty string there. After some discussion with @garfieldnate, it seems that this property is supposed to have values like "water" or "coffee" if there is a liquid there, and the 1.000000 is just a confidence level. When the object is empty, I suggest that V37 should be:

(V37 ^none 1.000000)

I believe this should be fixed in the Python code in Thor-Soar that puts things on the input link. That is not a Soar issue.

johnlaird commented 1 month ago

I agree with Peter. This should be a syntax error. Not high priority.


From: Nathan Glenn @.> Sent: Monday, July 22, 2024 7:22:34 PM To: SoarGroup/Soar @.> Cc: John Laird @.>; Mention @.> Subject: [SoarGroup/Soar] Soar doesn't quote empty string attribute names in outputs (Issue #484)

Source the following:

sp {foo (state ^superstate nil) --> ( ^|| hi) } step 1 p s1

This outputs the following:

(S1 ^ hi ^epmem E1 ^io I1 ^reward-link R1 ^smem L1 ^superstate nil ^type state)

We generally expect the output to be well-formed LHS syntax, but the bare caret will not parse if you try to use it in a production. This causes issues if you do any kind of processing of the output that then becomes input to Soar (e.g. constructing new productions from Soar output). It also wrongly suggests to the user that the bare caret syntax is legal.

A simple fix for this would be to quote empty string attribute names in the output of print.

@PLindeshttps://github.com/PLindes Doubts whether an empty attribute name should even be legal in Soar (deferring to @johnlairdhttps://github.com/johnlaird for a decision on that).

— Reply to this email directly, view it on GitHubhttps://github.com/SoarGroup/Soar/issues/484, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC4GS4EGJ25NDXNXVLEZHNTZNWH3VAVCNFSM6AAAAABLJHPEGGVHI2DSMVQWIX3LMV43ASLTON2WKOZSGQZDGOJSGQYDINA. You are receiving this because you were mentioned.Message ID: @.***>

johnlaird commented 1 month ago

I agree with Peter (again)! Could be empty instead of none, but none is probably correct.

John


From: Peter Lindes @.> Sent: Monday, July 22, 2024 7:35:06 PM To: SoarGroup/Soar @.> Cc: John Laird @.>; Mention @.> Subject: Re: [SoarGroup/Soar] Soar doesn't quote empty string attribute names in outputs (Issue #484)

I'm the one that originally brought up this issue, and from my perspective this is not addressing the real problem.

The original problem was in working with Thor-Soar. The Python code that takes data from AI2-Thor and puts a list of objects on the input link has a problem with a fill-liquid1 property. For the Bowl4 object it puts this on the input link:

p -d 3 o5 (O5 ^object-handle Bowl4 ^property P38 ^property P37 ^property P36 ^property P35 ^property P34 ^property P33 ^property P32 ^property P31 ^property P30 ^property P29 ^property P28 ^property P27 ^property P39) (P38 ^property-handle mass ^type visual ^values V38) (V38 ^|0.47| 1.000000) (P37 ^property-handle fill-liquid1 ^type visual ^values V37) (V37 ^ 1.000000) (P36 ^property-handle contents1 ^type visual ^values V36) (V36 ^empty1 1.000000) (P35 ^property-handle is-dirty1 ^type visual ^values V35) (V35 ^not-dirty1 1.000000) (P34 ^property-handle is-broken1 ^type visual ^values V34) (V34 ^not-broken1 1.000000) (P33 ^property-handle is-grabbed1 ^type visual ^values V33) (V33 ^not-grabbed1 1.000000) (P32 ^property-handle affordance1 ^type visual ^values V32) (V32 ^dirtyable1 1.000000) (P31 ^property-handle affordance1 ^type visual ^values V31) (V31 ^grabbable1 1.000000) (P30 ^property-handle receptacle ^type visual ^values V30) (V30 ^receptacle 1.000000) (P29 ^property-handle temperature ^type visual ^values V29) (V29 ^room-temp1 1.000000) (P28 ^property-handle is-reachable1 ^type visual ^values V28) (V28 ^not-reachable1 1.000000) (P27 ^property-handle category ^type visual ^values V27) (V27 ^bowl 1.000000) (P39 ^property-handle material ^type visual ^values V39) (V39 ^ceramic1 1.000000)

Notice this part:

(P37 ^property-handle fill-liquid1 ^type visual ^values V37) (V37 ^ 1.000000)

The attribute of the wme on V37 has no attribute name. This seems to me that it may be invalid Soar, but even if it is not I think there should be a non-empty string there. After some discussion with @garfieldnatehttps://github.com/garfieldnate, it seems that this property is supposed to have values like "water" or "coffee" if there is a liquid there, and the 1.000000 is just a confidence level. When the object is empty, I suggest that V37 should be:

(V37 ^none 1.000000)

I believe this should be fixed in the Python code in Thor-Soar that puts things on the input link. That is not a Soar issue.

— Reply to this email directly, view it on GitHubhttps://github.com/SoarGroup/Soar/issues/484#issuecomment-2243983820, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC4GS4AAARAAECTKAD2ZMELZNWJKVAVCNFSM6AAAAABLJHPEGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBTHE4DGOBSGA. You are receiving this because you were mentioned.Message ID: @.***>

garfieldnate commented 1 month ago

I think I need to clarify my thoughts here. My concern is not the syntax of productions, which is superficial; my concern is the underlying values that Soar works with (which I would say is the semantics).

First just a TL;DR for the estimations: adding the quotes to the print output would take less than an hour (fix this function) and would fix the issue that tripped up Peter, while making empty strings illegal as attribute names in Soar would be a huge effort (probably weeks to months with all of the design work, SML API rework, RHS function updates, migration documentation, etc.). Given the low priority, I only expect the former solution to be tenable.

Currently in Soar, there's only one real requirement for attribute names: they should be legal C-strings. Any valid Soar string can be used as an attribute name. This production, for example, sources without issue:

sp {foo
    (state <s> ^superstate nil)
-->
    (<s>
        ^|| hi
        ^| | hi
        ^|(| hi
        ^|\{| hi
        ^|*| hi
        ^|>| hi
        ^|=| hi
        ^|挨拶| hi
        ^|
| hi)
}

As long as the attribute names are quoted, you can use any valid c-string. This is easy to reason about because strings are a first-class type in Soar, along with floats, ints, identifiers and variables. If we disallowed attribute names from being the empty string (or any other otherwise valid string), we would essentially have two datatypes to keep track of: strings that are valid attribute names and strings that are not valid attribute names. This is because attribute names don't only come from parsing productions.

Attribute names can come from three other places that I know of:

  • Variables used as names in production RHS's:
sp {copy-val-to-name
    # imagine <bar> is the empty string
    (state <s> ^foo <bar>)
-->
    (<s>
        ^<bar> baz)
}
  • RHS functions
sp {rhs-function-to-att-name
    (state <s> ^superstate nil)
-->
    (<s>
        ^(concat || ||) hi)
}
  • The SML API:
my_id.CreateFloatWME("", 1.0)
my_id.CreateStringWME("", "hello")
# etc...

Are these attribute names weird? Sure, maybe. It depends on the what the programmer is making, which we the implementers cannot predict ahead of time. For example, maybe someone converts a database of first/last/middle names into a mapping using ^name <people-list>. A lot of people don't have middle names, so naturally the empty string might show up as an attribute name. If we disallowed the empty string as an attribute name, the programmer would get a runtime error telling them that they need to use some other string instead, and likely every programmer out there would come up with something different to avoid potential collision with other values in their application.

Given that attribute names can come from essentially any source and any arbitrary computation, outright disallowing empty strings would likely lead to many runtime errors for Soar users.

The (V37 ^ 1.000000) string that Peter pasted above is confusing and I can fix it quickly, which is why I opened this ticket.

How values in THOR-Soar are represented is a separate topic. However, I will say that the empty string attribute name shown above was not intentional on my part, and I've opened another ticket here on the issue that led to it.

johnlaird commented 1 month ago

Ok. I retract my suggestion. No reason to address empty strings in general, just fix Thor-Soar.


From: Nathan Glenn @.> Sent: Tuesday, July 23, 2024 12:42:24 AM To: SoarGroup/Soar @.> Cc: John Laird @.>; Mention @.> Subject: Re: [SoarGroup/Soar] Soar doesn't quote empty string attribute names in outputs (Issue #484)

I think I need to clarify my thoughts here. I'm not talking about syntax of productions; I'm talking about the underlying values that Soar works with (which I would say is the semantics).

First just a TL;DR for the estimations: adding the quotes to the print output would take less than an hour (fix thishttps://github.com/SoarGroup/Soar/blob/development/Core/SoarKernel/src/soar_representation/symbol.cpp#L80 function) and would fix the issue that tripped up Peter, while making empty strings illegal as attribute names in Soar would be a huge effort (probably weeks to months with all of the design work, SML API rework, RHS function updates, migration documentation, etc.).

Currently in Soar, there's only one real requirement for attribute names: they should be legal C-strings. Any valid Soar string can be used as an attribute name. This production, for example, sources without issue:

sp {foo (state ^superstate nil) --> ( ^|| hi ^| | hi ^|(| hi ^|{| hi ^|*| hi ^|>| hi ^|=| hi ^|挨拶| hi ^| | hi) }

As long as the attribute names are quoted, you can use any valid c-string. This is easy to reason about because strings are a first-class type in Soar, along with floats, ints, identifiers and variables. If we disallowed attribute names from being the empty string (or any other otherwise valid string), we would essentially have two datatypes to keep track of: strings that are valid attribute names and strings that are not valid attribute names. This is because attribute names don't only come from parsing productions.

Attribute names can come from three other places that I know of:

  • Variables used as names in production RHS's:

sp {copy-val-to-name

imagine is the empty string

(state <s> ^foo <bar>)

--> ( ^ baz) }

  • RHS functions

sp {rhs-function-to-att-name (state ^superstate nil) --> ( ^(concat || ||) hi) }

  • The SML API:

my_id.CreateFloatWME("", 1.0) my_id.CreateStringWME("", "hello")

etc...

Are these attribute names weird? Sure, maybe. It depends on the what the programmer is making, which we the implementers cannot predict ahead of time. For example, maybe someone converts a database of first/last/middle names into a mapping using ^name . A lot of people don't have middle names, so naturally the empty string might show up as an attribute name. If we disallowed the empty string as an attribute name, the programmer would get a runtime error telling them that they need to use some other string instead, and likely every programmer out there would come up with something different to avoid potential collision with other values in their application.

Given that attribute names can come from essentially any source and any arbitrary computation, outright disallowing empty strings would likely lead to many runtime errors for Soar users.

The (V37 ^ 1.000000) string that Peter pasted above is confusing and I can fix it quickly, which is why I opened this ticket.

How values in THOR-Soar are represented is a separate topic. However, I will say that the empty string attribute name shown above was not intentional on my part, and I've opened another ticket herehttps://github.com/SoarGroup/Soar/issues/485 on the issue that led to it.

— Reply to this email directly, view it on GitHubhttps://github.com/SoarGroup/Soar/issues/484#issuecomment-2244239850, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC4GS4BYKXQA5FS6GP2QRKLZNXNLBAVCNFSM6AAAAABLJHPEGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBUGIZTSOBVGA. You are receiving this because you were mentioned.Message ID: @.***>