timothygrant80 / cisTEM

Other
32 stars 26 forks source link

ConsoleTest::CtfNodes fails randomly #450

Open bHimes opened 1 year ago

bHimes commented 1 year ago

The console test for ctf nodes fails unpredictably. As discussed with @jojoelfe the final objective should be to fix the test rather than just loosening the test requirements.

An interim solution would be okay. I would suggest an option to add to any given console test that prints "SKIPPED" on a "FAIL" if it is a buggy test that requires further development. This is to be preferred to commenting out the test because it indicates the tested feature may be unreliable.

Further, suggest it be colored blue on the terminal.

jojoelfe commented 6 months ago

I think I have a lead. The problem is that CTF::Evaluate sometimes goes along the else path, indicating that low_resolution_contrast is not 0:

https://github.com/timothygrant80/cisTEM/blob/b7cf5feaaf7ce081d8dec438e4c2060a1ecadb03/src/core/ctf.cpp#L409C8-L419C10

Indeed in the cases where console_test fails with gcc, low_resolution_contrast is: 9508134425120630156296192.000000

A naive inspection makes one think low_resolution_contrast should always be 0:

https://github.com/timothygrant80/cisTEM/blob/b7cf5feaaf7ce081d8dec438e4c2060a1ecadb03/src/core/ctf.cpp#L33C1-L33C39

But the CTF constructor is heavily overloaded, and in many cases low_resolution_contrast is never assigned a value. This is undefined behavior. ICPC and Clang seem to be good about setting it to 0.0 by defualt, but gcc apparently does not. Maybe the easiest fix is to assign low_resolution_contrast a value in the header? Apparently it is used in Refine2D, where low_resolution_contrast is set to 0.5, for reasons that I don't understand at the moment.

bHimes commented 6 months ago

I think ensuring the default is set to zero is a good fix for now.

Looking over your clues, I think you are right. I very vaguely remember @timothygrant80 working on this to improve 2D classification prior to the intial cisTEM release.

It has voodoo hack written all over it.

If the tests are running okay, I would just add a note in the source, probably at the definition of low_resolution_contrast to the effect of

// ctf.h
.
.
.
float cubed_wavelength;
// Ensure default value is initialized. Only used in Refine2d a.t.m. See github issue #450 for discussion
// https://github.com/timothygrant80/cisTEM/issues/450
float low_resolution_contrast{0.f};

float squared_illumination_aperture;
float squared_energy_half_width;

public: