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

Update eds background #3044

Open
wants to merge 11 commits into
base: RELEASE_next_minor
Choose a base branch
from

Conversation

ZanettaPM
Copy link

@ZanettaPM ZanettaPM commented Oct 8, 2022

Description of the change

This is an updated version of PR #2836 . It is now rebased on 1.7.5

A new background component taking into account the detector efficiency, the composition of the sample and the edge jump is added to the components1D as an alternative to the Polynomial background.
This Background is based on Kramer emission law [Kramer 1923] (other emission models are also available) and convoluted with absorption model from Love and Scott [Sewell. et al, 1985] and the detector efficiency curve of the SEM/TEM.

Once this is merged it can easily take the output from PR #2535 and use detector efficiency curve for fitting the data.

Key words: Kramer's Law, Mass absorption mixture function, quantification, better fit

Progress of the PR

  • The component can be added to a model
  • The component is fitted with other components (gaussians)
  • Fix the fit_background() that free the fitting parameters problem
  • The fit works with datasets with 1,2 or 3 dimensions
  • The component can be exported, saved and reloaded in a new model
  • Add test
  • Correct typos in the documentation
  • Add refs in Bibliography
  • Ready for new reviews.

Example of how to fit the component

SEM example

s=hs.datasets.example_signals.EDS_SEM_Spectrum()
s.add_lines()
m=s.create_model(auto_background=False)
m.add_physical_background()
m.components.Physical_background.initialize() 
m.fit_background(bounded=True)
m.fit()
m.plot(plot_components=True)

TEM example

s = hs.datasets.example_signals.EDS_TEM_Spectrum()
s=s.isig[1.18:]
s.add_elements(['Ga','Cu','Si'])
s.add_lines()

kfactors = [1.77,1.450226,1,5.075602,1] #For Fe Ka and Pt La
bw = s.estimate_background_windows(line_width=[5.0, 2.0])
intensities = s.get_lines_intensity(background_windows=bw)
Weight_percent = s.quantification(intensities, method='CL', factors=kfactors,composition_units='weight' )

m=s.create_model(auto_background=False)
m.add_physical_background(quantification=Weight_percent) #here the detector is not the good one which explain the deviation at low energy
m.components.Physical_background.initialize()
m.fit_background(bounded=True) 
m.fit(bounded=True)
m.plot(plot_components=True)

@ZanettaPM
Copy link
Author

Hi all, sorry for creating a new PR one more time but I think this one is the good one (I hope).

If someone have time to review it, please tell me what you would like to change =).

@codecov
Copy link

codecov bot commented Oct 8, 2022

Codecov Report

Patch coverage: 51.89% and project coverage change: -0.27 ⚠️

Comparison is base (70ca076) 81.01% compared to head (54cb007) 80.74%.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #3044      +/-   ##
======================================================
- Coverage               81.01%   80.74%   -0.27%     
======================================================
  Files                     209      211       +2     
  Lines                   32806    33121     +315     
  Branches                 7540     7601      +61     
======================================================
+ Hits                    26579    26745     +166     
- Misses                   4469     4589     +120     
- Partials                 1758     1787      +29     
Impacted Files Coverage Δ
hyperspy/model.py 83.48% <16.66%> (-0.53%) ⬇️
hyperspy/_components/physical_background.py 48.82% <48.82%> (ø)
hyperspy/misc/eds/utils.py 79.09% <54.31%> (-13.44%) ⬇️
hyperspy/models/edsmodel.py 82.21% <80.00%> (-0.06%) ⬇️
hyperspy/misc/eds/detector_efficiency.py 100.00% <100.00%> (ø)
hyperspy/misc/material.py 84.00% <100.00%> (+0.21%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ZanettaPM
Copy link
Author

Hi all,
I need some help to finalize the physical background function. I have two main problems.
The first is that I see that now all components are defined as an "Expression". I am wondering if I need to change my code to align with this new way of organizing the function or not.
This is something I could do but it's not so simple in my case. The physical background is not just an expression because it takes arguments directly from the EDS model and it has to estimate the composition of each pixel before fitting the model to it. So, if I import the "physical background" component, I then need a step to create an array (that contain the composition) that is saved in the m.component.background._whitelist dictionary and is then used during the fitting to determine the mass absorption coefficient. Moreover, some of my parameters like the detector efficiency curve or the mass absorption coefficients are energy dependent and are already signals of equivalent size to the model signal and cannot be expressed as a function of energy as it is the case for other components. This also bring the problem of not being able to import this component in any model. If the signal is not an EDS signal, then I have to trick some of the previous written test to make it compatible. For instance, the simple test_creation_components1d() is a problem since it adds those components to a signal_1D model. On the other hand, is it really necessary to add an eds physical background to any kind of signal ?
If I do not have to convert that to an expression that would be easier for me, but I hope this is mergeable.
How this function works for now is

s=hs.datasets.example_signals.EDS_SEM_Spectrum()
s.add_lines()
m=s.create_model(auto_background=False)
m.add_physical_background()
m.components.Bremsstrahlung.initialize() 
m.fit_background(bounded=True)
m.fit()
m.plot(plot_components=True)

Where the initializing step allow to get arguments from the model and estimate the composition of each pixel before storing it in an array that is save in the dictionary.
I started to think that I could maybe use the super function in the edsmodel function to move the things up but I am not sure this would work.
My second problem is that because I save a lot of the parameters of the function in the m.component.background._whitelist I cannot extract the model as a dictionary. Some of the parameters return multiple value or text to the flags_str, value = value from the as_dictionnary function.
You can easily reproduce the error with

s=hs.datasets.example_signals.EDS_SEM_Spectrum()
s.set_lines([])
m = s.create_model(auto_background=False)
m.add_physical_background()
m.components.Bremsstrahlung.initialize()
m.fit_background(bounded=True)
m.fit(bounded=True)
x=m.as_dictionary()

or

s=hs.datasets.example_signals.EDS_SEM_Spectrum()
s.set_lines([])
m = s.create_model(auto_background=False)
m.add_physical_background()
x=m.as_dictionary()

I just want to make sure my code is in line with the coding style of hyperspy and I hope you will be able to help me on this particular point. So far it does run well (except for extracting it as a dictionary) which cause only one test to fail, but trying to go around this test made me think about all these questions.

@ericpre
Copy link
Member

ericpre commented Oct 13, 2022

Thanks @ZanettaPM for coming back to this! I hope that this time, it can be finished! Third time lucky? :)

The first is that I see that now all components are defined as an "Expression". I am wondering if I need to change my code to align with this new way of organizing the function or not. This is something I could do but it's not so simple in my case. The physical background is not just an expression because it takes arguments directly from the EDS model and it has to estimate the composition of each pixel before fitting the model to it. So, if I import the "physical background" component, I then need a step to create an array (that contain the composition) that is saved in the m.component.background._whitelist dictionary and is then used during the fitting to determine the mass absorption coefficient. Moreover, some of my parameters like the detector efficiency curve or the mass absorption coefficients are energy dependent and are already signals of equivalent size to the model signal and cannot be expressed as a function of energy as it is the case for other components. This also bring the problem of not being able to import this component in any model. If the signal is not an EDS signal, then I have to trick some of the previous written test to make it compatible. For instance, the simple test_creation_components1d() is a problem since it adds those components to a signal_1D model. On the other hand, is it really necessary to add an eds physical background to any kind of signal ? If I do not have to convert that to an expression that would be easier for me, but I hope this is mergeable.

Not all components are defined as Expression component, but doing so have advantages - this is discussed elsewhere. Generally, if this is possible to define component based on the Expression component, it is definitely the best solution.
The issue you described here is conceptual not a problem use expression, however there are a number if/else block in the calculation of the component which may be an issue. I haven't checked if this is actually a blocking issue, so I wouldn't comment any further.

For the failing test, this component should be treated separately as this is done with the EELSCLEdge component.

My second problem is that because I save a lot of the parameters of the function in the m.component.background._whitelist I cannot extract the model as a dictionary. Some of the parameters return multiple value or text to the flags_str, value = value from the as_dictionnary function.

I will need to look at it in more details, but this will need to be sorted.

@ZanettaPM
Copy link
Author

Ok I will start to think about a way to convert that as an expression, but this is probably something that we can update later after merging.

Ok I will treat this component in another test then. Let me do that quickly.

Yes, I started to look at the as_dictionary function, I think it should be easily fixed. It is just that I do not fully understand this function yet and why it is not sorting the parameters that are in the _whitelist

Thank you @ericpre for the help!

@ZanettaPM
Copy link
Author

Ok I think I fixed everything. I preferred to not touch to the whitelist exportation as it is already pretty well tuned for all components. Instead, few information are saved in m.components.Physical_background_dic. This small dictionary is exported in the dictionary returned by the m.as_dictionnary function and is reloaded in m._load_dictionary(dic).

@jlaehne
Copy link
Contributor

jlaehne commented Nov 13, 2022

however there are a number if/else block in the calculation of the component which may be an issue. I haven't checked if this is actually a blocking issue, so I wouldn't comment any further.

As far as I remember, one if/else block can be easily integrated in an expression component, but it is not simple for multiple. Might need something like an expression referencing another one.

On another note, would it be possible to increase the coverage further? There are a number of codecov warnings in the comments to the changed file, which would be best addressed directly in this PR.

@jlaehne jlaehne mentioned this pull request Nov 29, 2022
6 tasks
@ZanettaPM
Copy link
Author

On another note, would it be possible to increase the coverage further? There are a number of codecov warnings in the comments to the changed file, which would be best addressed directly in this PR.

I can work on it yes. I am just not sure how. I can add more test to cover the lines indicated. But I will have to find how to test it correctly.
Should it increase the coverage?
Thanks,

@jlaehne
Copy link
Contributor

jlaehne commented Nov 30, 2022

I can work on it yes. I am just not sure how. I can add more test to cover the lines indicated. But I will have to find how to test it correctly. Should it increase the coverage?

Yes, what is needed are additional test routines that make sure the indicated lines are executed by one of the tests and their intended use verified. Then, the calculated coverage should increase.

For lines, where an additional test makes no sense, a comment
# pragma: no cover can indicate this to the coverage analysis and it will be ignored in the calculation.

@ZanettaPM
Copy link
Author

Oh this is interesting to know thank you for the tips!

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