vpennella commented 8 years ago

It is working:

"melon".equals( "melon") true

and "melon" == "melon"

But if in the Set we have duplicate String like:

{"melon", "tomate", "melon"}.size()

3

Seems are couting the product "melon" one then more time..In this case the expected behaviour is to get 2 but we are getting 3.

matifreyre commented 8 years ago

I think this is not working because the underlying WSet doesn't compare by wollokEquals() but by the standard equals() when you add an object. Still don't know how to solve it "elegantly", though :-P

javierfernandes commented 8 years ago

Using a Set impl accepting a custom Comparator ?

El Saturday, June 25, 2016, Matías Freyre notifications@github.com escribió:

I think this is not working because the underlying WSet doesn't compare by wollokEquals() but by the standard equals() when you add an object. Still don't know how to solve it "elegantly", though :-P

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/762#issuecomment-228536874, or mute the thread https://github.com/notifications/unsubscribe/AEORWGayrv1XA32qgkgi4-lpMmgc3acWks5qPRnPgaJpZM4I8cPk .

npasserini commented 8 years ago

Yes, we should go that way and also we could fix the map implementation (I created an issue https://github.com/uqbar-project/wollok/issues/768).

On Sat, Jun 25, 2016 at 4:22 PM, javierfernandes notifications@github.com wrote:

Using a Set impl accepting a custom Comparator ?

El Saturday, June 25, 2016, Matías Freyre notifications@github.com escribió:

I think this is not working because the underlying WSet doesn't compare by wollokEquals() but by the standard equals() when you add an object. Still don't know how to solve it "elegantly", though :-P

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub < https://github.com/uqbar-project/wollok/issues/762#issuecomment-228536874 , or mute the thread < https://github.com/notifications/unsubscribe/AEORWGayrv1XA32qgkgi4-lpMmgc3acWks5qPRnPgaJpZM4I8cPk

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/762#issuecomment-228545506, or mute the thread https://github.com/notifications/unsubscribe/AEa1OcXF6gEZCRuTcFWEtzFEEfCTu4ezks5qPTmZgaJpZM4I8cPk .

fdodino commented 8 years ago

Ok, I think I can fix it, and also Map implementation.

fdodino commented 8 years ago

Mmm... added a Comparator, and this test failed:

def void occurrencesOfInSetsNotGreaterThanOne() {
    assert.equals(1, #{'Hola', 'mundo', 4, 4}.occurrencesOf(4))
    ...

That's natural, because now you have to compare elements using wollokEquals and wollokGreater... Should we expect to have such collections?

fdodino commented 8 years ago

Talking with @matifreyre he made me realize we should have a second opinion of wollokGreater. If we have two WKO, or classes not orderable, we should provide a catch for this situation.

javierfernandes commented 8 years ago

Well maybe I was confused. I said custom Comparator, but not because the order matters but because I thought that in that way the collection was not going to use equals and hashcode. The order shouldn't matter.

Now I'm thinking that I might be wrong that TreeSet still uses the equals and hashcode even if you change the Comparator ??

So, if possible use a Comparator that will only check for wollokEquals, if so, it will return 0, otherwise one of -1, or 1. Doesn't really matter which one. But I wouldn't force the collection to work with wollokGreater objects (comparable objects).

Otherwise if it still uses equals/hashcode, we should find a custom Set/Map that allows to customize the equals. I remember I did this many years ago for some other project. We created the "Equalator" or something interface to extract equals / hashcode into a strategy in the same way as a Comparator.

Cheers

On Mon, Jun 27, 2016 at 3:48 PM, Fernando Dodino notifications@github.com wrote:

Talking with @matifreyre https://github.com/matifreyre he made me realize we should have a second opinion of wollokGreater. If we have two WKO, or classes not orderable, we should provide a catch for this situation.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/762#issuecomment-228838791, or mute the thread https://github.com/notifications/unsubscribe/AEORWD_SCsQvfqCC26rJdzvs_5OmN65Wks5qQBr9gaJpZM4I8cPk .

fdodino commented 8 years ago

Ok!

fdodino commented 8 years ago

Mmmm... not so ok HashMap / HashSet doesn't have a Comparator at all, nor you can implement a custom "equalator". So I used a TreeMap / TreeSet. They need a Comparator based on compareTo method.

Now I reached this point

override compare(WollokObject o1, WollokObject o2) {
        if (o1.wollokEquals(o2)) {
            return 0
        }
        return o1?.toString.compareTo(o2?.toString)
    }

This implementation works for all existing test. This doesn't

override compare(WollokObject o1, WollokObject o2) {
        if (o1.wollokEquals(o2)) {
            return 0
        }
        return -1
    }

I prefer the first one...

javierfernandes commented 8 years ago

Yes, TreeSet works with comparator.

Cool, so it doesn't use the hash (logical based on names of impls)

Not sure why the test fails with the second impl. Could be "bad tests" relying on the order.

Anyway, the first solution seems fine for me !

:thumbs_up:

On Mon, Jun 27, 2016 at 4:21 PM, Fernando Dodino notifications@github.com wrote:

Mmmm... not so ok HashMap / HashSet doesn't have a Comparator at all, so I used a TreeMap / TreeSet. They need a Comparator based on compareTo method.

Now I reached this point

override compare(WollokObject o1, WollokObject o2) { if (o1.wollokEquals(o2)) { return 0 } return o1?.toString.compareTo(o2?.toString) }

This implementation works for all existing test. This doesn't

override compare(WollokObject o1, WollokObject o2) { if (o1.wollokEquals(o2)) { return 0 } return -1 }

I prefer the first one...

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/762#issuecomment-228847843, or mute the thread https://github.com/notifications/unsubscribe/AEORWJo70BEi0tpjMgpNjeHcCa3Q3ZExks5qQCLVgaJpZM4I8cPk .

fdodino commented 8 years ago

:smile: Created a PR

npasserini commented 8 years ago

Not sure why the test fails with the second impl. Could be "bad tests" relying on the order.

Yes there are things that depend on the order... I am not sure if it is right to say that they are "bad". The problem is that a TreeSet... depends on the order to look for objects into the tree. If you put objects into the tree that do not define a proper order relationship (transitive, reflexive, antisimmetric) ... the tree will not be able to find the objects you have put in the first place. So to use a TreeSet you HAVE to define an order.

Still... I think that the other implementation will also fail, because objects that have the same toString will return 0 from the comparator an therefore are considered equal by the tree.

Try this: pepa = new Golondrina()

s = new Dictionary() s.put(pepa, 0)

1..100.forEach{i => s.put(new Golondrina(), i)} // all objects are not == to pepa

assert.equals(s.get(pepa), 0) // wil faill I think.

// but assert.notEquals(pepa, new Golondrina()) assert.equals(s.size(), 101)

So... what to do? The cleaner way would be to implement our own set/map based on wollok equals (could be implemented fully in wollok?) or look for a native implementation that supports an "equalator" but not requiring an order. Or... hacky but I think it would work, base the order on something that cant produce false equal objects... do we have something like that? Is there an identityHashcode... but I think it could produce colisions. I am not sure, maybe the high way is the only one sometimes.

javierfernandes commented 8 years ago

identityHashCode from java shouldn't collide.

It is also available from wollok as "identity()" I believe.

On Mon, Jun 27, 2016 at 7:56 PM, Nico Passerini notifications@github.com wrote:

Not sure why the test fails with the second impl. Could be "bad tests" relying on the order.

Yes there are things that depend on the order... I am not sure if it is right to say that they are "bad". The problem is that a TreeSet... depends on the order to look for objects into the tree. If you put objects into the tree that do not define a proper order relationship (transitive, reflexive, antisimmetric) ... the tree will not be able to find the objects you have put in the first place. So to use a TreeSet you HAVE to define an order.

Still... I think that the other implementation will also fail, because objects that have the same toString will return 0 from the comparator an therefore are considered equal by the tree.

Try this: pepa = new Golondrina()

s = new Dictionary() s.put(pepa, 0)

1..100.forEach{i => s.put(new Golondrina(), i)} // all objects are not == to pepa

assert.equals(s.get(pepa), 0) // wil faill I think.

// but assert.notEquals(pepa, new Golondrina()) assert.equals(s.size(), 101)

So... what to do? The cleaner way would be to implement our own set/map based on wollok equals (could be implemented fully in wollok?) or look for a native implementation that supports an "equalator" but not requiring an order. Or... hacky but I think it would work, base the order on something that cant produce false equal objects... do we have something like that? Is there an identityHashcode... but I think it could produce colisions. I am not sure, maybe the high way is the only one sometimes.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/762#issuecomment-228900657, or mute the thread https://github.com/notifications/unsubscribe/AEORWLXWae9xgEowqkLx-1bzMhcZGksiks5qQFUngaJpZM4I8cPk .

npasserini commented 8 years ago

I am thinking that my idea is still wrong, even if the identityHashCode doesn't collide, it will not define an order relationship that is compatible with wollokEquals, so it won't work.

On Tue, Jun 28, 2016 at 1:07 AM, javierfernandes notifications@github.com wrote:

identityHashCode from java shouldn't collide.

It is also available from wollok as "identity()" I believe.

On Mon, Jun 27, 2016 at 7:56 PM, Nico Passerini notifications@github.com wrote:

Not sure why the test fails with the second impl. Could be "bad tests" relying on the order.

Yes there are things that depend on the order... I am not sure if it is right to say that they are "bad". The problem is that a TreeSet... depends on the order to look for objects into the tree. If you put objects into the tree that do not define a proper order relationship (transitive, reflexive, antisimmetric) ... the tree will not be able to find the objects you have put in the first place. So to use a TreeSet you HAVE to define an order.

Still... I think that the other implementation will also fail, because objects that have the same toString will return 0 from the comparator an therefore are considered equal by the tree.

Try this: pepa = new Golondrina()

s = new Dictionary() s.put(pepa, 0)

1..100.forEach{i => s.put(new Golondrina(), i)} // all objects are not == to pepa

assert.equals(s.get(pepa), 0) // wil faill I think.

// but assert.notEquals(pepa, new Golondrina()) assert.equals(s.size(), 101)

So... what to do? The cleaner way would be to implement our own set/map based on wollok equals (could be implemented fully in wollok?) or look for a native implementation that supports an "equalator" but not requiring an order. Or... hacky but I think it would work, base the order on something that cant produce false equal objects... do we have something like that? Is there an identityHashcode... but I think it could produce colisions. I am not sure, maybe the high way is the only one sometimes.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub < https://github.com/uqbar-project/wollok/issues/762#issuecomment-228900657 , or mute the thread < https://github.com/notifications/unsubscribe/AEORWLXWae9xgEowqkLx-1bzMhcZGksiks5qQFUngaJpZM4I8cPk

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/762#issuecomment-228902752, or mute the thread https://github.com/notifications/unsubscribe/AEa1ORHrLhx01CfFYeQhzt4vSJ6cWdrwks5qQFfNgaJpZM4I8cPk .

javierfernandes commented 8 years ago

Ok, a full map/set impl in wollok then :P Only using fold and without "return" on blocks. Won't be fast, but..

On Mon, Jun 27, 2016 at 8:15 PM, Nico Passerini notifications@github.com wrote:

I am thinking that my idea is still wrong, even if the identityHashCode doesn't collide, it will not define an order relationship that is compatible with wollokEquals, so it won't work.

On Tue, Jun 28, 2016 at 1:07 AM, javierfernandes <notifications@github.com

wrote:

identityHashCode from java shouldn't collide.

It is also available from wollok as "identity()" I believe.

On Mon, Jun 27, 2016 at 7:56 PM, Nico Passerini < notifications@github.com> wrote:

Not sure why the test fails with the second impl. Could be "bad tests" relying on the order.

Yes there are things that depend on the order... I am not sure if it is right to say that they are "bad". The problem is that a TreeSet... depends on the order to look for objects into the tree. If you put objects into the tree that do not define a proper order relationship (transitive, reflexive, antisimmetric) ... the tree will not be able to find the objects you have put in the first place. So to use a TreeSet you HAVE to define an order.

Still... I think that the other implementation will also fail, because objects that have the same toString will return 0 from the comparator an therefore are considered equal by the tree.

Try this: pepa = new Golondrina()

s = new Dictionary() s.put(pepa, 0)

1..100.forEach{i => s.put(new Golondrina(), i)} // all objects are not

to pepa

assert.equals(s.get(pepa), 0) // wil faill I think.

// but assert.notEquals(pepa, new Golondrina()) assert.equals(s.size(), 101)

So... what to do? The cleaner way would be to implement our own set/map based on wollok equals (could be implemented fully in wollok?) or look for a native implementation that supports an "equalator" but not requiring an order. Or... hacky but I think it would work, base the order on something that cant produce false equal objects... do we have something like that? Is there an identityHashcode... but I think it could produce colisions. I am not sure, maybe the high way is the only one sometimes.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub <

https://github.com/uqbar-project/wollok/issues/762#issuecomment-228900657

, or mute the thread <

https://github.com/notifications/unsubscribe/AEORWLXWae9xgEowqkLx-1bzMhcZGksiks5qQFUngaJpZM4I8cPk

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub < https://github.com/uqbar-project/wollok/issues/762#issuecomment-228902752 , or mute the thread < https://github.com/notifications/unsubscribe/AEa1ORHrLhx01CfFYeQhzt4vSJ6cWdrwks5qQFfNgaJpZM4I8cPk

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/762#issuecomment-228903998, or mute the thread https://github.com/notifications/unsubscribe/AEORWMX4vA8OUegoO30s-Sqz38-Ufystks5qQFl4gaJpZM4I8cPk .

fdodino commented 8 years ago

Yes, @npasserini , your test is right. It fails. :cry:

fdodino commented 8 years ago

Changed WollokObjectComparator, it will try to use wollok comparation. If not possible, will try to use hashCode comparation. Now this test works

    @Test
    def void addingSeveralSwallowsInAMap() {
        '''
        class Golondrina { }

        program a {
            var pepa = new Golondrina()
            var s = new Dictionary()
            s.put(pepa, 0)

            (1..100).forEach({i => s.put(new Golondrina(), i)}) // all objects are not == to pepa

            assert.notEquals(pepa, new Golondrina())
            assert.equals(101, s.size())
            assert.equals(0, s.get(pepa)) // will fail
        }
        '''.interpretPropagatingErrors

I know we should discuss about

better implementation for map & set objects (perhaps in Wollok)
or... redefine equals and hashCode in Wollok objects

But I think that we need to have a solution in order to teach main concepts of Set avoiding confusion to students.

uqbar-project / wollok

Set Collection is allowing to have duplicate string. #762

{"melon", "tomate", "melon"}.size()

1..100.forEach{i => s.put(new Golondrina(), i)} // all objects are not