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

Fix quant scaling error #2090

Merged

Conversation

k8macarthur
Copy link
Contributor

Description of the change

The dose calculation function used for cross section quantification previously assumed that the units of the navigation axis are in 'nm'. Therefore I've added a check to the function and convert_to_units for when the units are not 'nm'.

Test to check whether or not different units produces the same quantification results was also added.

As this is a bug fix no update to the user guide is needed.

@k8macarthur
Copy link
Contributor Author

This change will not change the units in the original spectrum image, just locally within the function for the correct electron dose to be calculated.

@ericpre ericpre changed the base branch from RELEASE_next_minor to RELEASE_next_patch December 1, 2018 16:39
@ericpre
Copy link
Member

ericpre commented Dec 1, 2018

@k8macarthur I have rebased your branch on Rnp to fix some left over of a merge conflict and the merge commits. You will need to checkout this branch.

Copy link
Member

@ericpre ericpre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things need to be fixed:

  • not to change the units in place,
  • currently get_dose("cross section") works only for spectrum image.

See more detailed comments below.

if self.axes_manager[0].units != 'nm':
self.axes_manager[0].convert_to_units('nm')
if self.axes_manager[1].units != 'nm':
self.axes_manager[1].convert_to_units('nm')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changing the units in place. The following should work:

pixel_size = []
# in case there are more than 2 navigation axes.
for axis in self.axes_manager.navigation_axes[:2]:                    
    pixel_size.append(axis.convert_to_units('nm', inplace=False)[0])

In particularly, for a dataset with navigation dimension > 2, how do we know if we pick the correct navigation axes? Here, I assume that the first two items of the axes_manager.navigation_axes are the "x" and "y" spatial dimension but it could something else!
Depending on how this is implemented, this should be documented.

if self.axes_manager[0].units != 'nm':
self.axes_manager[0].convert_to_units('nm')
if self.axes_manager[1].units != 'nm':
self.axes_manager[1].convert_to_units('nm')
pixel1 = self.axes_manager[0].scale
pixel2 = self.axes_manager[1].scale
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the method to get the dose will be wrong for linescan: self.axes_manager[1].scale could be a signal axis..

res2 = s2.quantification(intensities, method, factors)
np.testing.assert_allclose(res[0][0].data, res2[0][0].data)
# Check that the quantification doesn't change the units of the signal
assert s.axes_manager[0].units == 'µm'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is this assertion which need to be true. At the moment, the units are changed inplace.

@ericpre
Copy link
Member

ericpre commented May 17, 2019

I have fixed the comments of my review.

@francisco-dlp
Copy link
Member

LGTM

@francisco-dlp francisco-dlp merged commit cc2d49e into hyperspy:RELEASE_next_patch May 17, 2019
@francisco-dlp francisco-dlp added this to the v1.4.2 milestone May 21, 2019
@k8macarthur k8macarthur deleted the Fix_Quant_Scaling_Error branch July 1, 2019 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants