Skip to content

fix test-initial to allow more precise calculation#555

Merged
ZedThree merged 2 commits intoboutproject:masterfrom
dschwoerer:fix_test_initial
Jul 7, 2017
Merged

fix test-initial to allow more precise calculation#555
ZedThree merged 2 commits intoboutproject:masterfrom
dschwoerer:fix_test_initial

Conversation

@dschwoerer
Copy link
Contributor

I think this is the last fix needed for #551

@dschwoerer dschwoerer changed the base branch from next to master May 5, 2017 21:42
@dschwoerer dschwoerer mentioned this pull request May 5, 2017
@dschwoerer dschwoerer closed this May 5, 2017
@dschwoerer dschwoerer reopened this May 5, 2017
@ZedThree
Copy link
Member

To be honest, this change makes me a little uncomfortable, as I worry it might cause genuine bugs in BOUT++ to be missed. This is only a problem on one architecture which I'm not certain is used very much any more. I think this might be better solved by noting it as a "known bug" in the test README.md

@dschwoerer
Copy link
Contributor Author

I think this specific test is anyway not well designed.
For example, python does not guarantee a specific precision for floats. Therefore assuming that any python implementation has to use IEEE 754 seems not necessary.
What I think would be way better, is a test that makes sure that e.g. the BOUT PRNG actually produces uniform random numbers, and that the modes are on average initialized as they should. This however would mean a far more complicated, stochastic test.

The current test just makes sure that all the bugs from the C++ code where reproduced in python.
Sure, it is better then the old test, but this is imho not a test that convinces me that the code does what it is supposed to do ...

@bendudson
Copy link
Contributor

I think this is not going to be merged for now, since we're uneasy about making accuracy exceptions for particular tests. Perhaps just document that this failure is expected on some architectures

@bendudson bendudson closed this Jul 6, 2017
@dschwoerer
Copy link
Contributor Author

I would prefer getting the exception.
Having it documented is no use, if I want to run the check in the build process, as then I have to ignore the results (and thereby disable all checks) or not generate binaries for i686.

Further, I don't think the PRNG is decently tested ...

@dschwoerer
Copy link
Contributor Author

This is a histogram of the random output from using the python implementation of the bout's PRNG.
If we would care about this code - this should be fixed first.
bout_prng
This is the most basic test of the PRNG I could think of ... iterating a logistici map isn't a good idea if you want a good prng ...

So back to this PR:
The test is anyway not good, so allowing for it to not fail on higher precision machines shouldn't be our main issue ...

@dschwoerer dschwoerer reopened this Jul 6, 2017
@bendudson
Copy link
Contributor

Thanks for the testing. Yes, this does suck as a PRNG. Cryptographic use not recommended, but I put it in because it's simple and (I thought) reproducible. Do we need a better PRNG? I would prefer not to use a standard library because I don't think there's a way to make sure it will be the same in 5 year's time on a different system.

@bendudson
Copy link
Contributor

Presumably a PRNG which uses integers and then at the end generates a float should be more reproducible and less sensitive to floating point errors...?

@dschwoerer
Copy link
Contributor Author

Something like this maybe?
Thats the bad one from C++ 1

typedef unsigned int uint;
struct Ran{
  uint x;
  Ran(uint x_):x(x_){};
  uint unt(){
    x = x * 16807 % 2147483647;
    //printf("%d ",x);                                                                                                                                                            
    return x;
  }
  double flt(){
    return unt()*4.65661287524580e-10;
  }
};

It is better than what we have right know. It is not a good one - but should do the job.
The issue with C++ std suite seems to be, that the way the random numbers are converted to double/normal/... is not well defined 2

I think we would also be fine with std::mt19937 and provide our own code to create uniform/normal3/...4

@d7919 d7919 mentioned this pull request Jul 7, 2017
@d7919
Copy link
Member

d7919 commented Jul 7, 2017

There seem to be several possible issues here, but as I understand it the primary issue is that test-initial fails on i686 and that this means automated build systems that include the checks don't like it and abort (or similar).

Would a suitable workaround for now be to simply add some code into the specific runtest that just skips the mixmode test when on i686 (optionally with a warning)? That way we're not weakening the test for any other system but still allowing progress on i686. We can document the reason for the skip in the runtest and remove the skip should we address the somewhat separate issue (see #603 for example) of updating the mixmode modifier as several improvements are desirable but it is likely to break reproducibility.

@ZedThree ZedThree merged commit 9f67249 into boutproject:master Jul 7, 2017
@dschwoerer dschwoerer deleted the fix_test_initial branch February 27, 2018 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants