-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JMH test failure #538
Comments
How are you running tests? I'm running with |
I'm also running that way and it's also successful but if you look at the final results you won't see anything for this particular test (only 2/3 tests are shown). |
Hi Guys! (@rhyolight @khatchad)
First, this Logger error is (should be) a non-sequitur. As SLF4J allows each user to specify a log module to handle log output - when it isn't specified, this message is printed and a default logger is used. This is a setting inside of the JMH Benchmarking lib which I believe would have to specify this somehow... But this doesn't (shouldn't) affect the execution. Secondly I'm kind of confused as to what you might be expecting? When running with the "-Pskipbench" flag, the benchmarks won't run and you won't see any bench mark reports created in build/reports/jmh/* ? So that is the expected behavior. If you run with I'm also not clear on what test you say is missing? (i.e. what is the "this test" you say is missing) Also, how are you both "causing" the above exception, and where are you seeing it? |
Ah, sorry, I am not using |
Hi @cogmission,
Actually, what I was trying to bring attention to was the
Yes, agreed. Sorry about that. I was not using that flag as explained above. What I meant to say that my build was also showing
Yes, I do see it.
I mean to say that the benchmark
|
Hi @khatchad, Thank you, now I think I understand what the issue is to which you're bringing attention. Let me take a look at it. Also, thank you very much for hanging in there and explaining things to me! Cheers, |
No problem, @cogmission. Sorry I wasn't originally clear. |
I actually somehow fixed this problem by increasing the data size that the tests run on but I don't think you'd want that here. It seems to me that the intent of this test is a quick performance test. I'll post a pull request soon that will show this test passing (but taking much longer overall). |
#539 has a patch where the test does not fail. |
Hi @khatchad, Ok I've found the actual problem. It has to do with the transitivity of the comparator used for Global Inhibition in the SpatialPooler. There was an issue written up about this on StackOverflow... I set the comparator to: (to eliminate the rounding of values whose difference was < 0.000000001) and there is no error. I'm thinking about inserting this (p1, p2) -> {
double p1val = p1.getSecond();
double p2val = p2.getSecond();
return p1val == p2val ? 0 : p2val > p1val ? -1 : 1;
}; ...as opposed to the normal way... (p1, p2) -> {
int p1key = p1.getFirst();
int p2key = p2.getFirst();
double p1val = p1.getSecond();
double p2val = p2.getSecond();
if(Math.abs(p2val - p1val) < 0.000000001) {
return Math.abs(p2key - p1key) < 0.000000001 ? 0 : p2key > p1key ? -1 : 1;
} else {
return p2val > p1val ? -1 : 1;
}
}; I will take a look at submitting this as a pull request. I saw your pull request which increases the test size, and I really appreciate your effort on this, but I think inserting this will be a more "wholistic" solution? Here is a Gist with the class file to replace. Replace the AbstractAlgorithmBenchmark.java file found in src/jmh/java in the |
Hi @cogmission,
The increase of the test size in #539 is just to exploit more parallelism to show how the refactoring can optimize the code. So, yes, I agree with you that if changing the comparator solves the problem, it should be the solution given that this JMH test is supposed to be fast. |
I am seeing this failure when running the JHM tests. No results are produced from this test due to the failure:
The text was updated successfully, but these errors were encountered: