Re: [Jason-bugs] Fw: Anonymous variables are not renamed correctly. (public repost)

[Jomi H. <jom...@gm...>]

Dear Tim,

thanks for your careful analysis!

it isn't indeed a problem that your changes do not pass the jUnit  
tests. We have a bug to fix and its solution can break not well  
designed test (as you noted).

I am sorry for the lack of doc in the tests. (since ordinary users do  
not read java tests, we didn't give enough attention to doc in tests.)

I tend to agree you all your comments. But I still need to check it  
deeper. We also have ASUnit (AgentSpeak Unit tests) to test AS  
programs (and thus the interpreter). And your changes make some of  
them to fail. That case is worst because their are AS programs with a  
known and well defined expected execution.

If you want to run the ASUnit tests:
	cd applications/as-unit-test
	ant test

(I hope I have some time tomorrow to see the problem)

Thanks again,

Jomi

On 08/12/2009, at 02:37, Tim Cleaver wrote:

> Jomi,
>
> I have revisited the patch for anonymous variables. This time I have  
> implemented it against the current public svn version 1561. The  
> change to Structure.java is the same (although tidied up a bit).  
> This time, however, I have updated TermTest.java as well. These  
> changes aim not to change the intent of the tests, only the way
> they are carried out (although I may have made mistakes in  
> identifying their intent, given they aren't named descriptively nor  
> documented).


> Most of the changes revolve around moving from using toString to  
> testing the objects directly using equals and u.unifies. A side- 
> effect of this is that you can change the way the involved objects  
> are converted to strings without breaking the tests.

Yes. I agree.

>
> The test I had the most difficulty in understanding was  
> testMakeVarAnnon4(). It appears that a unifier is
> created, a manual renaming of one of the variables is made and it is  
> tested that this manual renaming is respected in makeVarsAnnon (i.e.  
> the variable isn't renamed further).

that's it.

> If this is the case, then the new test doesn't really capture that  
> desired behavior. I think, but maybe mistaken, that this behavior is  
> inconsistent with solving my original issue. Just because an unnamed  
> variable hasn't attained a value in the input unifier, does not mean  
> it shouldn't be renamed. Just because a named variable has been  
> renamed to an unnamed variable doesn't mean it does not need further  
> renaming. If you enforce renaming regardless of previous aliasing,  
> then the old renaming must be overridden. Thus, manual aliases  
> cannot be respected. If you ensure that you only rename a variable  
> once given an input unifier then situations where more than one  
> aliasing is
> necessary cannot be accommodated (like that situation identified in  
> my prior email).
>
> There is also an additional test that demonstrates the original  
> problem I was having. The original Structure.java will fail this  
> test. The patched version will not. The only downside to the new  
> Structure.java is that more anonymous variables are created than  
> previously (thus the removal of the count in testMakeVarAnnon3()).  
> Given the original problem was that not enough new variables were  
> created, I fail how to see this would be an issue.
>
> On a related issue, one of the original tests failed (with the new  
> Structure.java) because no unifier was passed into the makeVarsAnnon  
> method. Because makeVarsAnnon modifies the input unifier and an  
> empty default was supplied, the mapping from the old name to the new  
> was lost when the empty default unifier was discarded. Any attempt  
> at unification that does not use a unifier with the mapping from old  
> names to new will fail to assign values to the original variable.  
> Consequently, I believe that it would be beneficial to change the  
> interface of Literal.java to remove the makeVarsAnnon() method. This  
> would make it compulsory to pass in a unifier to this method. Given  
> clients won't know whether a given Literal will create new bindings  
> as a result of anonymizing the variables therein, clients don't know  
> whether to call makeVarsAnnon() or makeVarsAnnon(Unifier u). Most  
> will go with makeVarsAnnon() assuming it will Do the Right Thing,  
> not knowing they may lose
> information about variable aliases.
>
> Hopefully this provides more insight into the issue and that a  
> solution can be arrived at soon.
>
> regards,
> Tim
>
>
>       
> __________________________________________________________________________________
> See what's on at the movies in your area. Find out now: http://au.movies.yahoo.com/session-times/ 
> <UnnamedVarAnonymizeFix2.diff>

-- 
Jomi Fred Hubner
Department of Automation and Systems Engineering
Federal University of Santa Catarina
PO Box 476, Florianópolis, SC
88040-900 Brasil
http://www.das.ufsc.br/~jomi