dkriegner / xrayutilities

xrayutilities - a package with useful scripts for X-ray diffraction physicists
http://xrayutilities.sourceforge.io
GNU General Public License v2.0
81 stars 29 forks source link

update AlAs lattice parameter and Cij #165

Closed VeryBitter closed 1 year ago

VeryBitter commented 1 year ago

Hi Great library ! I found out the AlAs lattice parameter was inconsistent with our lab software database. I suggest to update it from this paper Appl. Phys. Lett. 66, 682–684 (1995): 5.66172 (instead of 5.6611). Our lab database indicates 5.66182 but I cannot find any reference yet.

dkriegner commented 1 year ago

thanks for the contribution. you are welcome to add yourself to the copyright statement at the top of the file.

dkriegner commented 1 year ago

/AzurePipelines run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
dkriegner commented 1 year ago

The last commit uploaded does not at all fit the scope of this pull request and should be removed.

I also do not like the way its implemented since FitModel is derived from lmfit.Model it should follow its way of using keyword arguments. So the method keyword argument to the class constructor seems to be against this pattern. I understand that already now I break the compatibility with lmfit.Model in some positions but I think it should not get worse. So the method must be passed to FitModel.fit and in fact this is already possible via the lmfit_kws argument of FitModel.fit. If you see a way to improve the situation and keep or improve the compatibility with lmfit.Model suggestions are welcome.

VeryBitter commented 1 year ago

Ooops

sorry, I think I commit my changes to the main repository, instead of my fork. I should have not. That was just some tests, sorry I am not well use to GitHub. I made another commit please delete it. I tried to use lmfit_kws but did not work. If you have any suggestion to do it properly, I want to switch from the method leastsq to differential_evolution. I also want to add an offset to incident angle in the dynamical model simulation, as a fitting parameter. Perhaps I should make an issue instead of coding myself.

I apologize for the troubles...

Aristide PS I downloaded the last version from GitHub, and I could change the method like this: fitr = fitmdyn.fit(Int, params, scanomcorr,fit_kws={'method':'differential_evolution'})

Le 3 juil. 2023 à 18:24, Dominik Kriegner @.***> a écrit :

The last commit uploaded does not at all fit the scope of this pull request and should be removed.

I also do not like the way its implemented since FitModel is derived from lmfit.Model it should follow its way of using keyword arguments. So the method keyword argument to the class constructor seems to be against this pattern. I understand that already now I break the compatibility with lmfit.Model in some positions but I think it should not get worse. So the method must be passed to FitModel.fit and in fact this is already possible via the lmfit_kws argument of FitModel.fit. If you see a way to improve the situation and keep or improve the compatibility with lmfit.Model suggestions are welcome.

— Reply to this email directly, view it on GitHub https://github.com/dkriegner/xrayutilities/pull/165#issuecomment-1618831522, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADC6OBUMTZEBX73FCOKMXGLXOLW3DANCNFSM6AAAAAAZ24O6BE. You are receiving this because you authored the thread.

VeryBitter commented 1 year ago

Hi I revert to the original models.py and fit.py files. I deeply apologize for all the mess.

dkriegner commented 1 year ago

I merged the change of the AlAs lattice parameters. Thanks for the contribution!

I have recently changed this FitModel part myself since I realized that not all keyword arguments can be passed through. I guess it should now work (either with fit_kws or lmfit_kws), but I am not entirely happy with the way its done. If you see problems please open a github issue and we come up with a solution.