mbuild icon indicating copy to clipboard operation
mbuild copied to clipboard

Replace decanexyz

Open rwsmith7531 opened this issue 4 years ago • 7 comments

PR Summary:

In testutils.py, test_structure_reproducibiliy would always xfail due to the decane.xyz standard file against which it would compare the newly-generated decane being outdated, as it was created with MDTraj instead of mbuild. This update now uses a standard file generated with the mbuild xyz writer instead. Due to the filename difference, the diff test was still failing, so the temp file was renamed to match the standard decane.xyz

PR Checklist


  • [x] Includes appropriate unit test(s)
  • [ ] Appropriate docstring(s) are added/updated
  • [x] Code is (approximately) PEP8 compliant
  • [ ] Issue(s) raised/addressed?

rwsmith7531 avatar Mar 04 '20 16:03 rwsmith7531

@rwsmith7531 and I worked together on this. The tests are passing on our machines 😂 . Im wondering if it is somehow using the old decane.xyz file still....

If someone can grab this branch and run the tests, im curious if this is somehow related to AZP.

justinGilmer avatar Mar 04 '20 17:03 justinGilmer

This was a headache when I last touched this code, and that is why I marked this test flaky: https://github.com/mosdef-hub/mbuild/pull/658#issuecomment-565052401

However, you'd still want to to use decane.xyz. The instructions for this test are

  1. Load a decane into memory
  2. Save it to 'decane.xyz'
  3. Compare the contents of 'decane.xyz' and 'decane-tmp.xyz'

If these file names are clobbered, the tests should pass automatically, since the comparison would presumably be against the same file. I'll also add that I should have named 'decane-tmp.xyz' something like 'decane-ref.xyz'

Also, for what it's worth, this test passes locally. I have yet to open the AZP logs but there may be a separate issue there.

mattwthompson avatar Mar 04 '20 17:03 mattwthompson

https://dev.azure.com/mosdef/mosdef/_build/results?buildId=904&view=logs&j=4bf9966a-0fce-5a84-b727-895eba4ceb25&t=66f48dc0-c1a0-5fef-b591-5ee2094f41e1

Just looking at the diffs in the two files one of them has -0.000000 and the other has 0.000000, which could be cause by floating point error somewhere (?)

E       AssertionError: assert not ['- C   -0.000000  -11.200000   -0.000000\n', '+ C   -0.000000  -11.200000    0.000000\n', '- H   -1.100000  -11.20000...1.200000    0.000000\n', '- C   -0.000000   -9.800000   -0.000000\n', '+ C   -0.000000   -9.800000    0.000000\n', ...]

uppittu11 avatar Mar 05 '20 15:03 uppittu11

I also wonder if it would be easier to use a simple molecule like water or methane. The diff on the positions of however many atoms in decane produces an error that's hard to read

mattwthompson avatar Mar 05 '20 16:03 mattwthompson

This is sort of relevant and kind of interesting https://stackoverflow.com/questions/4083401/negative-zero-in-python

uppittu11 avatar Mar 05 '20 16:03 uppittu11

Codecov Report

Merging #711 (1aa0776) into master (025abd4) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #711   +/-   ##
=======================================
  Coverage   89.74%   89.74%           
=======================================
  Files          63       63           
  Lines        8178     8178           
=======================================
  Hits         7339     7339           
  Misses        839      839           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 025abd4...1aa0776. Read the comment docs.

codecov[bot] avatar Apr 30 '21 18:04 codecov[bot]

In terms of getting this PR merged (2 years in the future), I think the simplest step is to follow @mattwthompson and just use a simpler molecule. @uppittu11 or @rwsmith7531 is this something you think you could give a try?

CalCraven avatar Aug 11 '22 17:08 CalCraven