[Bug]: Update topofileformats dependency to AFMReader [URGENT]

SylviaWhittle commented 3 weeks ago

Checklist

[X] Re-run analysis with topostats process --core 1.
[X] Describe the bug.
[X] Include the configuration file.
[X] Copy of the output.
[X] The exact command that failed. This is what you typed at the command line, including any options.
[X] TopoStats version, this is reported by topostats --version
[X] Operating System and Python Version

Describe the bug

Change the topofileformats dependency to AFMReader as it's currently broken.

Copy of the output

NA

Include the configuration file

NA

To Reproduce

NA

TopoStats Version

2.1.2

Python Version

3.11

Operating System

GNU/Linux

Python Packages

NA

ns-rse commented 3 weeks ago

Two tests fail on switching...

FAILED tests/test_io.py::test_load_scan_asd - assert -1368044348.3393068 == -71724923530211.84
FAILED tests/test_io.py::test_load_scan_get_data[load_scan_asd-197-image_shape5--673381139990.2344-file_122-2.0] - assert -12843725.967220962 == -673381139990.2344

Branch is ns-rse/851-afmreader.

I'll see if I can cross reference to tests in AFMReader tomorrow and work out what the correct values should be.

SylviaWhittle commented 3 weeks ago

😆

I have JUST found this as well!!

I was investigating right when you sent the message

I'll check if the kde plot of the heights makes sense

SylviaWhittle commented 3 weeks ago

First frame of the test asd file with its kde of heights

First frame of my topo_main environment that has topostats as of a few days ago installed:

See that the images are visually the same but the heights are scaled differently. The new heights are correct (or closer to correct) and the old ones are wrong - there's no shot that we have pixels in these images that are 50k tall.

@ns-rse I personally think that this shows that the new code is good to overwrite the test checksums?

ns-rse commented 3 weeks ago

Thanks @SylviaWhittle

Happy to go with your conclusion.

Would be useful to work out what the difference here is, perhaps topofileformats wasn't extracting the nanometer scaling correctly?

ns-rse commented 3 weeks ago

I've a virtualenv with topofileformats installed and have used [difftastic]() to compare asd.py between that and the same file from AFMReader.

I can see a few changes but am not sure these would have the magnitude of effect.

Where UniPolarConverter and BiPolarConverter are called the max_voltage variable is no longer passed to either, instead analogue_digital_range gets the value that was assigned to max_voltage, but this value goes from 2.0 (which appears to have been a tpyo) to 2.5 which looks like the correct value.

`topofileformats/asd.py`

712     elif analogue_digital_range == hex(0x00000002):
713         # unipolar 2.5V
714         mapping = (0.0, 2.5)
715         converter = UnipolarConverter(
716             analogue_digital_range=analogue_digital_range,
717             max_voltage=2.0,
718             resolution=resolution,                        
719             scaling_factor=scaling_factor,
720         )
...
739     elif analogue_digital_range == hex(0x00020000):
740         # bipolar 2.5V
741         mapping = (-2.5, 2.5)
742         converter = BipolarConverter(
743             analogue_digital_range=analogue_digital_range,
744             max_voltage=2.0,
745             resolution=resolution,
746             scaling_factor=scaling_factor,
747         )

`AFMREader/asd.py`

755     elif analogue_digital_range == hex(0x00000002):
756         # unipolar 2.5V
757         mapping = (0.0, 2.5)
758         converter = UnipolarConverter(
759             analogue_digital_range=2.5,
... 
760             resolution=resolution,
761             scaling_factor=scaling_factor,
762         )
...
786     elif analogue_digital_range == hex(0x00020000):
787         # bipolar 2.5V
788         mapping = (-2.5, 2.5)
789         converter = BipolarConverter(
790             analogue_digital_range=2.5,
... 
791             resolution=resolution,
792             scaling_factor=scaling_factor,
793         )

I noticed also that the sign is now negative in the UniPolarConverter.level_to_voltage() method (the calculation in BiPolarConverter.level_to_voltage() is unchanged).

`topofileformats/asd.py`

48     def level_to_voltage(self, level):
49         """Calculate the real world height scale in nanometres for an arbitrary level value.
..
50
51         Parameters
52         ----------
53         level: float
54             Arbitrary height measurement from the AFM that needs converting into real world
55             length scale units.
56
57         Returns
58         -------
59         float
60             Real world nanometre height for the input height level.
61         """
62         return (self.ad_range * level / self.resolution) * self.scaling_factor

`AFMReader/asd.py`

81     def level_to_voltage(self, level: float) -> float:
82         """
83         Calculate the real world height scale in nanometres for an arbitrary level value.
84 
85         Parameters
86         ----------
87         level : float
88             Arbitrary height measurement from the AFM that needs converting into real world
89             length scale units.
90 
91         Returns
92         -------
93         float
94             Real world nanometre height for the input height level.
95         """
96         multiplier = -self.ad_range / self.resolution * self.scaling_factor
97         return level * multiplier

Neither of these seem like they would result in the difference (new values are 52428.8 times smaller than old).

I don't think there needs to be any barrier to merging the changes in light of the manual check and so I will update the tests with the new values but thought it worth trying to identify the underlying cause.

AFM-SPM / TopoStats