Skip to content
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

inconsistent results between Mac and Linux #18

Open
kjappelbaum opened this issue Dec 17, 2022 · 9 comments
Open

inconsistent results between Mac and Linux #18

kjappelbaum opened this issue Dec 17, 2022 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@kjappelbaum
Copy link

as far as i understand this one of the reasons the test suite for the conda recipe is failing for OSX (https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=628582&view=logs&j=58ac6aab-c4bc-5de2-2894-98e408cc8ec9&t=933f325c-924e-533d-4d95-e93b5843ce8b)

For some reason, for instance, the ASA / A^2 in the reference is 60.7713 but on Mac 59.1876

Not really sure where the difference comes from, however, we should already be a bit saner if we have a test suite that does not depend on rounding. Perhaps we should just have some simple parsers and then use pytest.approx

@kjappelbaum kjappelbaum self-assigned this Dec 17, 2022
@kjappelbaum
Copy link
Author

kjappelbaum commented Dec 19, 2022

I can reproduce the error in the test suite when I manually compile on an m1 mac

>       assert sa == sa_ref
E       AssertionError: assert {'ASA_A^2': 5...^2/cm^3', ...} == {'ASA_A^2': 6...^2/cm^3', ...}
E         Omitting 20 identical items, use -vv to show
E         Differing items:
E         {'ASA_m^2/g': 1186.46} != {'ASA_m^2/g': 1218.21}
E         {'ASA_m^2/cm^3': 1924.9} != {'ASA_m^2/cm^3': 1976.4}
E         {'Channel_surface_area_A^2': 59.1876} != {'Channel_surface_area_A^2': 60.7713}
E         {'ASA_A^2': 59.1876} != {'ASA_A^2': 60.7713}
E         Use -v to get more diff

the difference is more than 2%, hard to ignore it.

@kjappelbaum kjappelbaum changed the title don't use diff in test suite inconsistent results between Mac and Linux Dec 19, 2022
@kjappelbaum
Copy link
Author

and volpo is even worse

        for key in volpo:
>           assert volpo[key] == pytest.approx(volpo_ref[key], rel=0.05)
E           assert 23.787 == 22.6493 ± 1.1e+00
E             comparison failed
E             Obtained: 23.787
E             Expected: 22.6493 ± 1.1e+00

@kjappelbaum
Copy link
Author

it is, however, really hard for me to understand the problem. The only clue I have is to perhaps try with different versions of Eigen

@kjappelbaum kjappelbaum added the bug Something isn't working label Dec 19, 2022
@kjappelbaum
Copy link
Author

also the PSD bin counts are off

@ltalirz
Copy link
Member

ltalirz commented Dec 19, 2022

Do the algorithms involve any kind of (pseudo-)randomness?

If the seed / random number generator is different on the two platforms, that could be another reason for the discrepancy.
In that case, one should allow for an appropriate uncertainty in the test result (which is now trivial to add as you migrated to pytest).

@kjappelbaum
Copy link
Author

kjappelbaum commented Dec 19, 2022

Thanks! Didn't check this so far. But it is certainly important to also note this in the docs. Currently, I use pytest.approx with tolerances that pass on my Mac ... But the discrepancy >5% seemed a bit large to me

@ltalirz
Copy link
Member

ltalirz commented Dec 19, 2022

Does the discrepancy reduce when you increase the number of samples?

@kjappelbaum
Copy link
Author

kjappelbaum commented Dec 19, 2022

good point, and thanks for the feedback and ideas.

Didn't test systematically so far. At least for the PSD I didn't get it to match with 5000000 samples (but I'm sure that this algorithm involves randomness and one should evaluate it more carefully, also making plots). For the SA, things seem to improve with more samples.
This is also the only thing that is now failing in the conda recipe.

@ltalirz
Copy link
Member

ltalirz commented Dec 19, 2022

For PSD, it might be worth checking that it is not related to #5

zeo++/psd.cc:822:15: error: Uninitialized variable: volFraction [uninitvar]
          if (volFraction >= TOL) {

Edit: Ok, this looks like a false alarm

zeopp-lsmo/zeo++/psd.cc

Lines 814 to 821 in d641dbf

if (size) {
diff = 2 * Radius2 - overlapDist;
volFraction = volIntersect / vol2;
}
if (!size) {
diff = 2 * Radius1 - overlapDist;
volFraction = volIntersect / vol1;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants