Moses-support Digest, Vol 104, Issue 87

Send Moses-support mailing list submissions to
moses-support@mit.edu

To subscribe or unsubscribe via the World Wide Web, visit
http://mailman.mit.edu/mailman/listinfo/moses-support
or, via email, send a message with subject or body 'help' to
moses-support-request@mit.edu

You can reach the person managing the list at
moses-support-owner@mit.edu

When replying, please edit your Subject line so it is more specific
than "Re: Contents of Moses-support digest..."


Today's Topics:

1. Weird code in Hypothesis::RecombineCompare() (Jeroen Vermeulen)
2. Re: Weird code in Hypothesis::RecombineCompare()
(Marcin Junczys-Dowmunt)
3. Re: Weird code in Hypothesis::RecombineCompare() (Hieu Hoang)
4. Re: Weird code in Hypothesis::RecombineCompare()
(Jeroen Vermeulen)
5. Re: Weird code in Hypothesis::RecombineCompare() (Hieu Hoang)


----------------------------------------------------------------------

Message: 1
Date: Thu, 25 Jun 2015 15:09:56 +0700
From: Jeroen Vermeulen <jtv@precisiontranslationtools.com>
Subject: [Moses-support] Weird code in Hypothesis::RecombineCompare()
To: "moses-support@mit.edu" <moses-support@mit.edu>
Message-ID: <558BB754.6090607@precisiontranslationtools.com>
Content-Type: text/plain; charset=utf-8

Looking at replacing WordsBitmap's implementation with std::vector<bool>
(less code, less memory) I came across this function:

?
/** check, if two hypothesis can be recombined.
this is actually a sorting function that allows us to
keep an ordered list of hypotheses. This makes recombination
much quicker.
*/
int
Hypothesis::
RecombineCompare(const Hypothesis &compare) const
{
// -1 = this < compare
// +1 = this > compare
// 0 = this ==compare
int comp = m_sourceCompleted.Compare(compare.m_sourceCompleted);
if (comp != 0)
return comp;

for (unsigned i = 0; i < m_ffStates.size(); ++i) {
if (m_ffStates[i] == NULL || compare.m_ffStates[i] == NULL) {
comp = m_ffStates[i] - compare.m_ffStates[i];
} else {
comp = m_ffStates[i]->Compare(*compare.m_ffStates[i]);
}
if (comp != 0) return comp;
}

return 0;
}
?


My problem is with this conditional subtraction:

?
if (m_ffStates[i] == NULL || compare.m_ffStates[i] == NULL) {
comp = m_ffStates[i] - compare.m_ffStates[i];
?

The result of that subtraction looks technically undefined to me, in
which case _theoretically_ I could replace it with anything I liked
including code to recite Homer in Morse code on the hard-disk light.
But what is it meant to do in practice?

The assignment to comp casts the value from std::ptrdiff_t to int. On a
two's-complement system with 64-bit pointers, 32-bit ints, and a zero
null pointer, the could would boil down to: take either m_ffStates[i] or
~m_ff_states[i] + 1 depending on which one is non-null, divide it by the
size of FFState, drop the most-significant 32 bits, and compare the
least-signficant 32 bits to zero. Occasionally you may get a completely
unexpected zero even though one of the pointers is non-null, but usually
you'll get an arbitrary positive or negative result. On the other hand
it wouldn't surprise me if the optimizer were allowed to make convenient
assumptions about the truncation, and just re-use the sign of the
original ptrdiff_t.

Am I right in thinking this comparison is more or less arbitrary, as
long as the result is consistent and only zero if the two pointers are
both null? If so, would anyone mind if I made it compare just the
nullness of the two pointers?

The actual code may not end up looking like this, but I'm thinking along
the lines of:

?
comp = (
int(m_ffStates[i] == NULL) - int(compare.m_ffStates[i] == NULL)
);
?

This way, a null pointer would be deterministically "less than" a
non-null pointer.


Jeroen


------------------------------

Message: 2
Date: Thu, 25 Jun 2015 10:13:58 +0200
From: Marcin Junczys-Dowmunt <junczys@amu.edu.pl>
Subject: Re: [Moses-support] Weird code in
Hypothesis::RecombineCompare()
To: Jeroen Vermeulen <jtv@precisiontranslationtools.com>
Cc: moses-support@mit.edu
Message-ID: <8d57c93821942129bef60c14013477d4@amu.edu.pl>
Content-Type: text/plain; charset="utf-8"



I'm pretty sure that's the "major" bug we missed until now ;)

W dniu 2015-06-25 10:09, Jeroen Vermeulen napisa?(a):

> Looking at replacing WordsBitmap's implementation with std::vector<bool>
> (less code, less memory) I came across this function:
>
> ?
> /** check, if two hypothesis can be recombined.
> this is actually a sorting function that allows us to
> keep an ordered list of hypotheses. This makes recombination
> much quicker.
> */
> int
> Hypothesis::
> RecombineCompare(const Hypothesis &compare) const
> {
> // -1 = this < compare
> // +1 = this > compare
> // 0 = this ==compare
> int comp = m_sourceCompleted.Compare(compare.m_sourceCompleted);
> if (comp != 0)
> return comp;
>
> for (unsigned i = 0; i < m_ffStates.size(); ++i) {
> if (m_ffStates[i] == NULL || compare.m_ffStates[i] == NULL) {
> comp = m_ffStates[i] - compare.m_ffStates[i];
> } else {
> comp = m_ffStates[i]->Compare(*compare.m_ffStates[i]);
> }
> if (comp != 0) return comp;
> }
>
> return 0;
> }
> ?
>
> My problem is with this conditional subtraction:
>
> ?
> if (m_ffStates[i] == NULL || compare.m_ffStates[i] == NULL) {
> comp = m_ffStates[i] - compare.m_ffStates[i];
> ?
>
> The result of that subtraction looks technically undefined to me, in
> which case _theoretically_ I could replace it with anything I liked
> including code to recite Homer in Morse code on the hard-disk light.
> But what is it meant to do in practice?
>
> The assignment to comp casts the value from std::ptrdiff_t to int. On a
> two's-complement system with 64-bit pointers, 32-bit ints, and a zero
> null pointer, the could would boil down to: take either m_ffStates[i] or
> ~m_ff_states[i] + 1 depending on which one is non-null, divide it by the
> size of FFState, drop the most-significant 32 bits, and compare the
> least-signficant 32 bits to zero. Occasionally you may get a completely
> unexpected zero even though one of the pointers is non-null, but usually
> you'll get an arbitrary positive or negative result. On the other hand
> it wouldn't surprise me if the optimizer were allowed to make convenient
> assumptions about the truncation, and just re-use the sign of the
> original ptrdiff_t.
>
> Am I right in thinking this comparison is more or less arbitrary, as
> long as the result is consistent and only zero if the two pointers are
> both null? If so, would anyone mind if I made it compare just the
> nullness of the two pointers?
>
> The actual code may not end up looking like this, but I'm thinking along
> the lines of:
>
> ?
> comp = (
> int(m_ffStates[i] == NULL) - int(compare.m_ffStates[i] == NULL)
> );
> ?
>
> This way, a null pointer would be deterministically "less than" a
> non-null pointer.
>
> Jeroen
> _______________________________________________
> Moses-support mailing list
> Moses-support@mit.edu
> http://mailman.mit.edu/mailman/listinfo/moses-support [1]



Links:
------
[1] http://mailman.mit.edu/mailman/listinfo/moses-support
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mailman.mit.edu/mailman/private/moses-support/attachments/20150625/446b8dac/attachment-0001.htm

------------------------------

Message: 3
Date: Thu, 25 Jun 2015 12:41:57 +0400
From: Hieu Hoang <hieuhoang@gmail.com>
Subject: Re: [Moses-support] Weird code in
Hypothesis::RecombineCompare()
To: moses-support@mit.edu
Message-ID: <558BBED5.4070804@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed

i don't think m_ffStates[i] can be null and compare.m_ffStates[i] can
ever be NOT null.

it should be an assert of UTIL_THROW_IF(...), rather than an if statement

On 25/06/2015 12:09, Jeroen Vermeulen wrote:
> Looking at replacing WordsBitmap's implementation with std::vector<bool>
> (less code, less memory) I came across this function:
>
> ?
> /** check, if two hypothesis can be recombined.
> this is actually a sorting function that allows us to
> keep an ordered list of hypotheses. This makes recombination
> much quicker.
> */
> int
> Hypothesis::
> RecombineCompare(const Hypothesis &compare) const
> {
> // -1 = this < compare
> // +1 = this > compare
> // 0 = this ==compare
> int comp = m_sourceCompleted.Compare(compare.m_sourceCompleted);
> if (comp != 0)
> return comp;
>
> for (unsigned i = 0; i < m_ffStates.size(); ++i) {
> if (m_ffStates[i] == NULL || compare.m_ffStates[i] == NULL) {
> comp = m_ffStates[i] - compare.m_ffStates[i];
> } else {
> comp = m_ffStates[i]->Compare(*compare.m_ffStates[i]);
> }
> if (comp != 0) return comp;
> }
>
> return 0;
> }
> ?
>
>
> My problem is with this conditional subtraction:
>
> ?
> if (m_ffStates[i] == NULL || compare.m_ffStates[i] == NULL) {
> comp = m_ffStates[i] - compare.m_ffStates[i];
> ?
>
> The result of that subtraction looks technically undefined to me, in
> which case _theoretically_ I could replace it with anything I liked
> including code to recite Homer in Morse code on the hard-disk light.
> But what is it meant to do in practice?
>
> The assignment to comp casts the value from std::ptrdiff_t to int. On a
> two's-complement system with 64-bit pointers, 32-bit ints, and a zero
> null pointer, the could would boil down to: take either m_ffStates[i] or
> ~m_ff_states[i] + 1 depending on which one is non-null, divide it by the
> size of FFState, drop the most-significant 32 bits, and compare the
> least-signficant 32 bits to zero. Occasionally you may get a completely
> unexpected zero even though one of the pointers is non-null, but usually
> you'll get an arbitrary positive or negative result. On the other hand
> it wouldn't surprise me if the optimizer were allowed to make convenient
> assumptions about the truncation, and just re-use the sign of the
> original ptrdiff_t.
>
> Am I right in thinking this comparison is more or less arbitrary, as
> long as the result is consistent and only zero if the two pointers are
> both null? If so, would anyone mind if I made it compare just the
> nullness of the two pointers?
>
> The actual code may not end up looking like this, but I'm thinking along
> the lines of:
>
> ?
> comp = (
> int(m_ffStates[i] == NULL) - int(compare.m_ffStates[i] == NULL)
> );
> ?
>
> This way, a null pointer would be deterministically "less than" a
> non-null pointer.
>
>
> Jeroen
> _______________________________________________
> Moses-support mailing list
> Moses-support@mit.edu
> http://mailman.mit.edu/mailman/listinfo/moses-support

--
Hieu Hoang
Researcher
New York University, Abu Dhabi
http://www.hoang.co.uk/hieu



------------------------------

Message: 4
Date: Thu, 25 Jun 2015 15:55:39 +0700
From: Jeroen Vermeulen <jtv@precisiontranslationtools.com>
Subject: Re: [Moses-support] Weird code in
Hypothesis::RecombineCompare()
To: moses-support@mit.edu
Message-ID: <558BC20B.8080508@precisiontranslationtools.com>
Content-Type: text/plain; charset=utf-8

On 25/06/15 15:41, Hieu Hoang wrote:
> i don't think m_ffStates[i] can be null and compare.m_ffStates[i] can
> ever be NOT null.
>
> it should be an assert of UTIL_THROW_IF(...), rather than an if statement

That's a little beyond my ken though... I'm nervous enough about
touching code here that isn't even legal in the first place. :-)

How about I just leave a TODO comment saying to look into it? Maybe you
could then add the assert later if you're interested.


Jeroen


------------------------------

Message: 5
Date: Thu, 25 Jun 2015 12:57:54 +0400
From: Hieu Hoang <hieuhoang@gmail.com>
Subject: Re: [Moses-support] Weird code in
Hypothesis::RecombineCompare()
To: Jeroen Vermeulen <jtv@precisiontranslationtools.com>
Cc: moses-support <moses-support@mit.edu>
Message-ID:
<CAEKMkbi3Udsf9tmpNq2tfnqkWW4HYdqO0tood7SnbZHmQ2T18A@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"

sure, someone might get round to looking at it

Hieu Hoang
Researcher
New York University, Abu Dhabi
http://www.hoang.co.uk/hieu

On 25 June 2015 at 12:55, Jeroen Vermeulen <
jtv@precisiontranslationtools.com> wrote:

> On 25/06/15 15:41, Hieu Hoang wrote:
> > i don't think m_ffStates[i] can be null and compare.m_ffStates[i] can
> > ever be NOT null.
> >
> > it should be an assert of UTIL_THROW_IF(...), rather than an if statement
>
> That's a little beyond my ken though... I'm nervous enough about
> touching code here that isn't even legal in the first place. :-)
>
> How about I just leave a TODO comment saying to look into it? Maybe you
> could then add the assert later if you're interested.
>
>
> Jeroen
> _______________________________________________
> Moses-support mailing list
> Moses-support@mit.edu
> http://mailman.mit.edu/mailman/listinfo/moses-support
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mailman.mit.edu/mailman/private/moses-support/attachments/20150625/2fe7998a/attachment.htm

------------------------------

_______________________________________________
Moses-support mailing list
Moses-support@mit.edu
http://mailman.mit.edu/mailman/listinfo/moses-support


End of Moses-support Digest, Vol 104, Issue 87
**********************************************

Tweet