search

MDAnalysis / UserGuide

User Guide for MDAnalysis
https://userguide.mdanalysis.org
22 stars 31 forks source link

Rerun diffusion_map.ipynb so tests pass #345

Closed pgbarletta closed 7 months ago

pgbarletta commented 7 months ago

Fixes #342

Error showed up on 14/11, after passing on 10/11. The only commit on the main MDA repo during that time was on the 11/11, which refactored distance calculation. DiffusionMap uses DistanceMatrix, which I assume uses the distances functions exposed through cython.

Given that the sign change in an eigenvector is not relevant and that we found (or at least I think so), where the change came from, I think we can just update the stored notebook so the test pass again.

:books: Documentation preview :books:: https://mdanalysisuserguide--345.org.readthedocs.build/en/345/

review-notebook-app[bot] commented 7 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

IAlibay commented 7 months ago

The slight modification to the distance backend shouldn't have been anything else than cosmetic - we did get this issue popping up irregularly before, I wonder if it's some upstream change.

In any case - @RMeli @lilyminium as leads on documentation could one of you please triage this?

RMeli commented 7 months ago

Thanks @lilyminium for taking care of this. The sign change is not relevant, and can be caused by many things, such as changes in the underlying eigensolver implementation.

IAlibay commented 7 months ago

Maybe to clarify my comment re: sign change & upstream - it's not that I don't agree that it's not relevant, but rather that it's probably not going to consistently be one way or the other (edit: because it's not behaviour we introduced). I.e. it may be worth considering if we'll be here with another failure next week just because things flipped.

pgbarletta commented 7 months ago

Ok, if it breaks again, next time I'll ignore the output of that cell.

Il dom 10 dic 2023, 07:59 Irfan Alibay @.***> ha scritto:

Maybe to clarify my comment re: sign change & upstream - it's not that I don't agree that it's not relevant, but rather that it's probably not going to consistently be one way or the other. I.e. it may be worth considering if we'll be here with another failure next week just because things flipped.

— Reply to this email directly, view it on GitHub https://github.com/MDAnalysis/UserGuide/pull/345#issuecomment-1848930254, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLOAK7OX4DBUOZQ6XHSSADYIWI2VAVCNFSM6AAAAABADBSG3CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBYHEZTAMRVGQ . You are receiving this because you were mentioned.Message ID: @.***>