search

ReliaSolve / cctbx_project

Computational Crystallography Toolbox
https://cctbx.github.io
Other
0 stars 0 forks source link

Fix probe vs. reduce dot gap calculation discrepancy #248

Closed russell-taylor closed 1 year ago

russell-taylor commented 1 year ago

Laptop code is up to date with rlabduke/probe master branch. The desktop code is using the reliasolve master branch for reduce (e9d54ed7c9edf6844b0b95c120b10f0e9a5322d3). There was a bug introduced into one of the distance calculations in reduce along the way. Putting it back to the original calculation makes them agree again.

Well... that was in version 4.11 that claimed to be fixing a bug introduced in version 4.7. All of this is modifying the dot-interaction codes. Need to look into this to see which version is correct. The "bug" that was introduced may in fact be the correct behavior.

The current probe master branch calculations use the distance from the dot to the second atom and subtract the radius of the second atom, which matches the distance2(q, locb) calculation. This is using a gapSize() function to determine the gap and there are no code differences in the line that calls gapSize() going back to before I touched the code (9b198c1d1bb2cbf19cd404d372294836bda8ed0a) -- it is still using the dot location rather than the probe location.

Just before the 4.7 change (2a87ad9c8ab142b12774e616e052ab10be38a1ba), reduce used sqrt(squaredist), which is using distance from the probe. Back before I touched it in 2018 at 57b3dbe6e920f45820c21afed5fba29f607961ce, it was also using sqrt(squaredist), which is the distance from the probe to the second atom, which does not match original probe's calculations.

russell-taylor commented 1 year ago

Version 4.7 generalized the code to work when the probe radius was nonzero. The two were the same in the case where it is not. When we made the probe radius default to larger than 0, this caused the discrepancy in the code to show up and be a problem.

russell-taylor commented 1 year ago

(done) Verify that with probe radius 0 we get the expected behavior from the new reduce without the code modification. This will help validate that we fixed a problem because it will not have broken anything.

russell-taylor commented 1 year ago

Fixed in version 4.13.