Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
7 tasks done
SylviaWhittle opened this issue Jun 5, 2024 · 5 comments · Fixed by #852
Closed
7 tasks done

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

SylviaWhittle opened this issue Jun 5, 2024 · 5 comments · Fixed by #852
Labels
bug Something isn't working

Comments

@SylviaWhittle
Copy link
Collaborator

Checklist

  • Re-run analysis with topostats process --core 1.
  • Describe the bug.
  • Include the configuration file.
  • Copy of the output.
  • The exact command that failed. This is what you typed at the command line, including any options.
  • TopoStats version, this is reported by topostats --version
  • 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

@SylviaWhittle SylviaWhittle added the bug Something isn't working label Jun 5, 2024
@ns-rse
Copy link
Collaborator

ns-rse commented Jun 5, 2024

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
Copy link
Collaborator Author

😆

I have JUST found this as well!!

I was investigating right when you sent the message

image

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

@SylviaWhittle
Copy link
Collaborator Author

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

image image

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

image image

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
Copy link
Collaborator

ns-rse commented Jun 6, 2024

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
Copy link
Collaborator

ns-rse commented Jun 6, 2024

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.

ns-rse added a commit that referenced this issue Jun 6, 2024
As detailed in #851 the sum of the arrays changes on switching from `topofileformats` > `AFMReader`. The issue notes
some details of investigation.
ns-rse added a commit that referenced this issue Jun 6, 2024
As detailed in #851 the sum of the arrays changes on switching from `topofileformats` > `AFMReader`. The issue notes
some details of investigation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants