search

dealii / dealii

The development repository for the deal.II finite element library
https://www.dealii.org
Other
1.4k stars 751 forks source link

FunctionParser still uses boost::random #9177

Closed bangerth closed 4 years ago

bangerth commented 4 years ago

9125 alerts me to the fact that the source/base/function_parser.cc file still uses the BOOST random number facilities. #9125 moves this functionality to source/base/mu_parser_internal.cc, so we should wait till that PR has been merged, but after that, we should replace the BOOST random facilities by the corresponding C++11 facilities.

@konsim83 -- FYI.

rezarastak commented 4 years ago

Given this code comment, do you think we should check and ensure the consistency of the standard random generator implementations across all compilers?

https://github.com/dealii/dealii/blob/b66c8af5cdd18b7121a6a6882217e7da9e586a57/source/base/mu_parser_internal.cc#L130-L135

bangerth commented 4 years ago

I personally don't think it's important to have consistency in this case and would much rather get rid of the BOOST dependency. User is requesting random numbers, user gets random numbers, however they are generated.

So I'd just as well go with what you have in #9204. What do others think?

masterleinad commented 4 years ago

We reverted #8312 in https://github.com/dealii/dealii/pull/8332 that addressed the same issue since we just had too many problems with failing tests.

rezarastak commented 4 years ago

Thank you @masterleinad for directing us to the history of these types of changes.

8312 was a very large change and it was affecting a lot of tests. Perhaps if we apply the same changes in smaller patches it is an achievable task.

It seems that the C++11 random number generators are fully portable (except std::default_random_engine and std::random_device), but the distributions might be platform dependent (link). So one idea is that we define our own random distribution, which replicates the implementation of its boost counterpart. This way, hopefully, we can remove dependence on boost::random and simultaneously keep old tests passing without modification.

masterleinad commented 4 years ago

I am happy with all changes that replace boost functions by corresponding ones in the C++ standard library and pass the test suite for all configurations. If it is just about updating the output files, that is also OK, but with compiler/platform-dependent output it gets pretty nasty.

In the end, I agree with @bangerth that it should really matter which random numbers are returned by these functions and therefore it might not be a good idea to print random numbers in test output at all. On the other hand, it is likely infeasible to modify all corresponding tests accordingly.

I don't really see an advantage defining our own random distributions instead of the ones provided by boost, though. We are not going to get rid of the boost dependency in the near future and boost::random has worked pretty fine so far.

tjhei commented 4 years ago
  1. I agree with @masterleinad that there is no inherent problem with using boost.
  2. I don't think this particular change is as big of a problem (we only use the function parser random functionality in one test).
  3. We likely need to add alternative outputs (or disable printing the random number).
rezarastak commented 4 years ago

Okay I will change the tests to not print the random numbers, instead the tests will check general properties of the generated numbers.

bangerth commented 4 years ago

I think as a general principle, if there is C++11 functionality that we can use instead of using BOOST, then that's what we should do. I do agree that that sometimes is not possible for practical reasons.

I do wonder whether the C++11 standard specifies how exactly things like mt19937 are supposed to work. It specifies the general algorithm, and I suspect also the initializer? If so, then we shouldn't incur any dependency on the actual compiler used.

bangerth commented 4 years ago

By the way, scroll all the way to the bottom of https://en.cppreference.com/w/cpp/numeric/random/mersenne_twister_engine and you will find two very interesting notes:

The 10000th consecutive invocation of a default-constructed std::mt19937 is required 
to produce the value 4123659995.

The 10000th consecutive invocation of a default-constructed 
std::mt19937_64 is required to produce the value 9981545732273789042

So I think we can assume that the std:: random number generators generate numbers in a platform-independent way.

rezarastak commented 4 years ago

Yes, I read that the random-number generators are platform-independent. However, the random number distributions such as std::uniform_real_distribution can be implementation dependent.

bangerth commented 4 years ago

Oh :-( I missed that part. Let's hope that we don't get into too much trouble. I think it was still the right thing to do, and probably a good idea to not actually output random numbers.