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

New example for fitting MBE grown heterostructure #166

Merged
merged 24 commits into from
Jul 18, 2023

Conversation

VeryBitter
Copy link
Contributor

I had a bit of a struggle before making a good modeling and fitting of my experimental results.
I would like to add another example file so that newcomers could use it to fit their data within a short amount of time.
I hope it will be helpful.

Aristide

Found a newer value for AlAs lattice parameter, closer to our lab's value
Update AlAs lattice parameter and Cij
Deleting unwanted commit file
I had a bit of a struggle before making a good modeling and fitting of my experimental results.
I would like to add another example file so that newcomers could use it to fit their data within a short amount of time.
I hope you will consider it.
update AlAs lattice parameter and Cij (dkriegner#165)
@dkriegner
Copy link
Owner

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dkriegner dkriegner self-requested a review July 5, 2023 15:24
Copy link
Owner

@dkriegner dkriegner left a comment

Choose a reason for hiding this comment

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

hi,
thanks for the contribution. sorry for the large amount of comments. I hope they do not discourage you. I only try to keep a certain consistency in the style.

please look at the linter errors at
https://dev.azure.com/dominikkriegner/xrayutilities/_build/results?buildId=515&view=logs&j=011e1ec8-6569-5e69-4f06-baf193d1351e&t=c0307c8e-cb29-5919-a192-498b9fd8bfd8

Also if possible avoid setting things in the example which are the default anyways. Examples get too long otherwise and the essential things can be missed.

When running your example with lmfit version 1.2.1 I get the following error:

TypeError                                 Traceback (most recent call last)
File .../simpack_xrd_dyn_AlGaAs.py:150
    147 lmfit_kws = {'nan_policy':'omit'}
    149 # launch fitting procedure
--> 150 fitr = fitmdyn.fit(Int, params, om_off, fit_kws = fit_kws, lmfit_kws = lmfit_kws)
    153 # get Al composition for perpendicular lattice parameter
    154 x_Al = x_Al(fitr.params['AlAs_0_80_GaAs_0_20__c'], GaAs_.a, AlGaAs80)

File .../xrayutilities/simpack/fit.py:268, in FitModel.fit(self, data, params, x, weights, fit_kws, lmfit_kws, **kwargs)
    264         self.fitplot.updatemodelline(kwargs['x'],
    265                                      self.eval(params, **kwargs))
    267 # perform fitting
--> 268 res = super().fit(data[mask], params, x=x[mask], weights=mweights,
    269                   fit_kws=fit_kws, iter_cb=cb_func, **lmfit_kws)
    271 # final update of plot
    272 if self.fitplot.plot:

File /usr/lib/python3.11/site-packages/lmfit/model.py:1086, in Model.fit(self, data, params, weights, method, iter_cb, scale_covar, verbose, fit_kws, nan_policy, calc_covar, max_nfev, **kwargs)
   1083 if fit_kws is None:
   1084     fit_kws = {}
-> 1086 output = ModelResult(self, params, method=method, iter_cb=iter_cb,
   1087                      scale_covar=scale_covar, fcn_kws=kwargs,
   1088                      nan_policy=self.nan_policy, calc_covar=calc_covar,
   1089                      max_nfev=max_nfev, **fit_kws)
   1090 output.fit(data=data, weights=weights)
   1091 output.components = self.components

TypeError: lmfit.model.ModelResult() got multiple values for keyword argument 'method'

What could be different to your setup? I think this is not an error in your example but some problem in FitModel, but we should also solve it.

examples/data/simpack_xrd_AlGaAs.xrdml Outdated Show resolved Hide resolved
examples/simpack_xrd_dyn_AlGaAs.py Outdated Show resolved Hide resolved
examples/simpack_xrd_dyn_AlGaAs.py Outdated Show resolved Hide resolved
examples/simpack_xrd_dyn_AlGaAs.py Outdated Show resolved Hide resolved
examples/simpack_xrd_dyn_AlGaAs.py Outdated Show resolved Hide resolved
examples/simpack_xrd_dyn_AlGaAs.py Outdated Show resolved Hide resolved
examples/simpack_xrd_dyn_AlGaAs.py Outdated Show resolved Hide resolved
examples/simpack_xrd_dyn_AlGaAs.py Outdated Show resolved Hide resolved
examples/simpack_xrd_dyn_AlGaAs.py Outdated Show resolved Hide resolved
@VeryBitter
Copy link
Contributor Author

Hi
I did all the cleaning requested. I was able to use AlGaAs.ContentBtsym.
Regarding the "values for keyword argument 'method'" error, I reinstalled xrayutilities, downgraded lmfit to 1.2.0, still working. I don't understand why it doesn't work on your configuration. Actually, I was able to use successfully the fit_kws = {'method': 'differential_evolution'} parameter only after you updated the fit.py file very recently.

@VeryBitter VeryBitter requested a review from dkriegner July 6, 2023 13:09
@dkriegner
Copy link
Owner

thanks for all the fixes. I'll need a bit of time to come back to you. Please stay tuned.

/AzurePipelines run

update Changelog with changes from dkriegner#165
@dkriegner
Copy link
Owner

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Owner

@dkriegner dkriegner left a comment

Choose a reason for hiding this comment

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

It looks already good. I still get the same error as before. I suggest to add this new example to the tests/test_examples.py file so that its also automatically run in the tests. We will see then if the error can be reproduced there. I think it could be an issue in FitModel.

If the Azure tests succeed with this example included than it would be a local issue on my end.

examples/simpack_xrd_dyn_AlGaAs.py Outdated Show resolved Hide resolved
examples/simpack_xrd_dyn_AlGaAs.py Outdated Show resolved Hide resolved
Co-authored-by: Dominik Kriegner <dominik.kriegner@gmail.com>
@VeryBitter
Copy link
Contributor Author

Hi Dominique
do you expect me to do something at this point? I fully agree with your suggestion.

@dkriegner
Copy link
Owner

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dkriegner
Copy link
Owner

Hi Dominique do you expect me to do something at this point? I fully agree with your suggestion.

Hi, I think I just performed the remaining changes. let's see what the tests are saying now.

@dkriegner
Copy link
Owner

Unfortunately it seems the unit tests have the same error I experience locally. I look into it.

@dkriegner
Copy link
Owner

I have a fix for the issue with the method keyword argument. Basically one has to pass it via the lmfit_kws instead of fit_kws. But before I upload it I have a question: Why you want to use method='differential_evolution'. In my tests the script needs 30 times longer with differential_evolution than when using leastsq. Also the resulting chi-square is slightly worse. So I see no reason to use a special optimization method here. What am i missing?

@VeryBitter
Copy link
Contributor Author

I first used the leastsq method. But the fit was not that good, looking like it was stuck in secondary minimum. Switching to differential_evolution gave me much better results. However, based on you comment, setting the min/max for each parameter might have be the true reason for the fit improvement. So probably we could comment this line. Although not necessary, it would give a hint on how to change the fit model and other parameters. Or remove it completely. Up to you.

@dkriegner
Copy link
Owner

dkriegner commented Jul 17, 2023 via email

@dkriegner
Copy link
Owner

/AzurePipelines run

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 166 in repo dkriegner/xrayutilities

@dkriegner
Copy link
Owner

/AzurePipelines run

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 166 in repo dkriegner/xrayutilities

@dkriegner dkriegner merged commit 0af8aa7 into dkriegner:main Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants