Opened 14 years ago

Closed 14 years ago

Last modified 13 years ago

#2703 closed Bugs (fixed)

lagged_fibonacci_01::seed() bug

Reported by: rick68@… Owned by: No-Maintainer
Milestone: Boost 1.38.0 Component: random
Version: Boost 1.37.0 Severity: Problem
Keywords: Cc: Jens.Maurer@…, rick68@…

Description

Hi,

In <boost/random/lagged_fibonacci_01.hpp>

line309: template<class It>
line310: void seed(It& first, It last)
line311: {
line312:#ifndef BOOST_NO_STDC_NAMESPACE
line313: allow for Koenig lookup
line314: using std::fmod;
line315: using std::pow;
line316:#endif[[BR]] line317: unsigned long mask = ~((~0u) << (w%32));
now lowest w bits set
line318: RealType two32 = pow(RealType(2), 32);
line319: unsigned int j;
line320: for(j = 0; j < long_lag && first != last; ++j, ++first) {
line312: x[j] = RealType(0);
line313: for(int k = 0; k < w/32 && first != last; ++k, ++first)
line314: x[j] += *first / pow(two32,k+1);
line315: if(first != last && mask != 0)
line316: x[j] += fmod((*first & mask) / _modulus, RealType(1));
line317: }
line318: i = long_lag;
line319: if(first == last && j < long_lag)
line320: throw std::invalid_argument("lagged_fibonacci_01::seed");
line321: }

line 313 maybe rewrite to:

for(int k = 0; k < w/32 && first != last; ++k)

because of the first "for loop" can't be broken.

Attachments (2)

lagged_fibonacci.hpp.diff (519 bytes ) - added by rick68@… 14 years ago.
Patch
lagged_fibonacci.hpp.diff2 (534 bytes ) - added by rick68@… 14 years ago.
First patch file maybe has mistake. I put second patch file.

Download all attachments as: .zip

Change History (8)

by rick68@…, 14 years ago

Attachment: lagged_fibonacci.hpp.diff added

Patch

comment:1 by Steven Watanabe, 14 years ago

Resolution: invalid
Status: newclosed

This is not a bug. The value_type of the iterator is assumed to be a 32 bit integer, while RealType is usually a double. The loop is needed because RealType may have more bits of precision than the iterator's value_type. Thus, more than one element of the input range may be needed to initialize a single element of the state. Therefore the iterator needs to be incremented in the inner loop.

in reply to:  1 comment:2 by rick68@…, 14 years ago

Cc: rick68@… added
Resolution: invalid
Status: closedreopened

Replying to steven_watanabe:

This is not a bug. The value_type of the iterator is assumed to be a 32 bit integer, while RealType is usually a double. The loop is needed because RealType may have more bits of precision than the iterator's value_type. Thus, more than one element of the input range may be needed to initialize a single element of the state. Therefore the iterator needs to be incremented in the inner loop.

Hi,

I tried this test code:

================================================================================
#include <iostream>
#include <vector>
#include <stdexcept>
#include <boost/cstdlib.hpp>
#include <boost/cstdint.hpp>
#include <boost/random/lagged_fibonacci.hpp>
int main(void) {

typedef std::vector<boost::int32_t> seed_type;

boost::lagged_fibonacci607 gen;
seed_type seed(gen.long_lag);
seed_type::iterator first;
seed_type::iterator second;

std::cout << "(second - first) = [" << (second - first) << ']' << std::endl;

first = seed.begin();
second = seed.end();
std::cout << "(second - first) = [" << (second - first) << ']' << std::endl;

try {

gen.seed(first, second);

} catch (const std::invalid_argument& e) {

std::cout << e.what() << std::endl;

}

std::cout << "(second - first) = [" << (second - first) << ']' << std::endl;

return boost::exit_success;

} ================================================================================

And this is my result:
=================================
(second - first) = [0]
(second - first) = [607]
(second - first) = [-607]
=================================

I can't catch any exception, and member function "gen.seed()" will out of range.

comment:3 by rick68@…, 14 years ago

Hi,

In <boost/random/subtract_with_carry.hpp>, I found member fucntion substract_with_carry::seed(), it is similar lagged_fibonacci_01::seed().

line295: template<class It>
line296: void seed(It& first, It last)
line297: {
line298:#ifndef BOOST_NO_STDC_NAMESPACE
line299: allow for Koenig lookup
line300: using std::fmod;
line301: using std::pow;
line302:#endif[[BR]] line303: unsigned long mask = ~((~0u) << (w%32));
now lowest (w%32) bits set
line304: RealType two32 = pow(RealType(2), 32);
line305: unsigned int j;
line306: for(j = 0; j < long_lag && first != last; ++j) {
line307: x[j] = RealType(0);
line308: for(int i = 0; i < w/32 && first != last; ++i, ++first)
line309: x[j] += *first / pow(two32,i+1);
line310: if(first != last && mask != 0) {
line311: x[j] += fmod((*first & mask) / _modulus, RealType(1));
line312: ++first;
line313: }
line314: }
line315: if(first == last && j < long_lag)
line316: throw std::invalid_argument("subtract_with_carry_01::seed");
line317: carry = (x[long_lag-1] ? 0 : 1 / _modulus);
line318: k = 0;
line319: }

Maybe lagged_fibonacci_01::seed() has mistake.

by rick68@…, 14 years ago

Attachment: lagged_fibonacci.hpp.diff2 added

First patch file maybe has mistake. I put second patch file.

comment:4 by Steven Watanabe, 14 years ago

Ok. I see. Yes, it's the outer increment that needs to be removed. This is probably too late for 1.38, but it should be in 1.39.

comment:5 by Steven Watanabe, 14 years ago

Resolution: fixed
Status: reopenedclosed

(In [51120]) correctly detect the end of the range in lagged_fibonacci_01::seed. Fixes #2703

comment:6 by buy Cialis side effects interactions, 13 years ago

Note: See TracTickets for help on using tickets.