seeding fix for xvalmmd

[vigsterkr]

fix #4783

[vigsterkr]

@lambday this change currently breaks some mmd unit tests :(
would just simply updating to new values be sufficient?
only prng changes here...

[lambday]

@vigsterkr for CrossValidationMMD unit-tests, the checks are more fundamental it seems, cannot just update to the new values. Is the internal SGObject also using std::mt19937_64 prng?

[vigsterkr]

yep it's the same prng all over

[vigsterkr]

@lambday note that if i would just add there 2 dummy calls for the prng, before creating the folds objects, things would pass...

[vigsterkr]

@lambday i'm pretty sure that if i would do prng(); prng();
this patch would pass

[vigsterkr]

@lambday in fact i've just tested it locally and the hack i've written actually makes all the unit tests pass that are currently failing... so this is just the problem of having the right expected value/seed

[lambday]

@vigsterkr adding dummy calls to the main codebase might not be the ideal way to go here I think. Would it be possible to create two different prng instances in the unit-test rather? e.g. one instance to pass to the CrossValidationMMD guy here and another one to pass to the explicit testing logic here. Both should have the same seed. I think that might solve the problem as well?

[lambday]

For the other failing unit-test, we can just update to the new value that it gets.

[vigsterkr]

Not so sure I understand the rational for this separate prng...? And why not just to update the seed or expected value? I mean it does not use one global prng, but objects that are nested and have prngs there the prng is shared... but eg the data generators in the unit test and xvalmmd doesn’t share one prng...

…

On 24 Oct 2019, at 05:31, Soumyajit De ***@***.***> wrote:  @vigsterkr adding dummy calls to the main codebase might not be the ideal way to go here I think. Would it be possible to create two different prng instances in the unit-test rather? e.g. one instance to pass to the CrossValidationMMD guy here and another one to pass to the explicit testing logic here. Both should have the same seed. I think that might solve the problem as well? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[lambday]

@vigsterkr for KernelSelectionMaxCrossValidation unit-tests, we can just update the expected value with the new value we're getting here.

But it's a bit tricky for the CrossValidationMMD unit-test. This test does not use a constant as the expected value. Rather, it computes the expected value independently using PermutationMMD within the test itself, and compares that value with that we get from the xval class.

Now, these two independent computations would yield the same result, if we feed each of them the same class of prng with the same state.

But as per the present logic, we are using the same prng instance to compute one after another - these two computations are not independent anymore. This is why the these are not giving the same result I am afraid.

I think if we can just create two instances of the prng with the same seed in these unit-tests, pass one to the shogun CrossValidationMMD class and pass the other one to the explicit computation logic in the test to the PermutationMMD class (like this), then the tests would pass.

[vigsterkr]

@lambday this is now how it works that we just pass on things with prngs :)
but note since
https://github.com/shogun-toolbox/shogun/blob/develop/tests/unit/statistical_testing/internals/CrossValidationMMD_unittest.cc#L110
https://github.com/shogun-toolbox/shogun/blob/develop/tests/unit/statistical_testing/internals/CrossValidationMMD_unittest.cc#L94

these guys dont share a prng.... just saying... so then jsut updating the results should be fine right?

[vigsterkr]

@lambday i guess this is what u had in mind 7022a56

note that every test in CrossValidationMMD unit tests fails atm with this change, but since the only check is

EXPECT_EQ(cv.m_rejections(current_run*num_folds+current_fold, k), p_value<alpha);

i'm not so sure what is update-able here...?

the other test that fails is KernelSelectionMaxCrossValidation.quadratic_time_single_kernel_dense, but i guess there the constant could be just updated to its new value:

694: The difference between selected_kernel->get_width() and 0.03125 is 0.09375, which exceeds 1E-10, where
694: selected_kernel->get_width() evaluates to 0.125,
694: 0.03125 evaluates to 0.03125, and
694: 1E-10 evaluates to 1e-10.

[vigsterkr]

note that if i revert the change in CrossValidationMMD but keep the prng changes you've requested, there's still one test that fails: CrossValidationMMD.unbiased_incomplete

[vigsterkr]

@lambday any ideas? since now we have 2 same type but separate prng seeded with the same seed... but the unit test still fails :(

[lambday]

@vigsterkr let me check this out.. worst case scenario, we're gonna remove the explicit computation logic and put an array of constant expected values... but I want to understand why these are still not giving the same result..

[vigsterkr]

@lambday any updates on this by any chance?

[lambday]

@vigsterkr hi! I tried to reproduce the issue in my local and it seems like the issue is solved?

[gf712]

@vigsterkr is this still an issue?

[vigsterkr]

yep

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.