cctbx / cctbx_project

Computational Crystallography Toolbox
https://cci.lbl.gov/docs/cctbx
Other
207 stars 111 forks source link

Gauss star gonio #985

Open dermen opened 2 months ago

dermen commented 2 months ago

This includes two big additions to diffBragg and modeling

The Delta-Q falloff was suggested by James here, which fixed the issue we were observing with F_latt for some space groups.. however still in the process of determining exactly what goes in the exponential..

nksauter commented 2 months ago

I'd like to make sure that the pure-kokkos version of exascale_api supports this new feature. Probably need a short call about this.

Baharis commented 2 months ago

You are saying that "a perfectly round RELP called gauss_star [...] eliminates issues where F_latt varies with symmetry operations". Which is great, because it should fix C- and F- centered cells; however, this PR changes the way Na, NB, and Nc, numbers of unit cells in microcrystal, are used, doesn't it? Does it mean the new behaviour is to 1) take Na, Nb, Nc lattice parameters in each direction in the primitive cell as before or 2) the phil-supplied (i.e. primitive or non-primitive) cell and somehow scale intensities if necessary?

dermen commented 2 months ago

@nksauter definitely happy to meet this week- I think it still needs some testing, not quite ready to merge it in yet.. There is a new test called simtbx/tests/tst_api_congruency.py which checks the (hopefully identical) behavior of the various simulators ..

@Baharis , the new model is limited compared to the original gaussian shape. We can discuss.. But I believe Na,Nb,Nc are meant to correspond to numbers of cells along the primitive lattice. I need to add a change to diffBragg_cpu_kernel.cpp to reflect this..

Baharis commented 2 months ago

@Baharis , the new model is limited compared to the original gaussian shape. We can discuss.. But I believe Na,Nb,Nc are meant to correspond to numbers of cells along the primitive lattice. I need to add a change to diffBragg_cpu_kernel.cpp to reflect this.

Ok, if that is the case then I unfortunately do not understand the problem this PR is solving. Apparently, it solves issues with simulating some space groups, but as long as Na,Nb,Nc correspond to numbers of cell along the primitive lattice, I- and F-centered cells will be always simulated wrong.

As for the "full support of goniometer rotation in diffBragg", does it mean diffBragg can be now used to simulate rotation data in addition to stills?

dermen commented 2 months ago

@Baharis, to elaborate, the new model only depends on the product of NaNbNc , so it might not be the most realistic..

dermen commented 2 months ago

and yes, adding the gonio allows to refine rotation data, however the current code does not allow refinement of the goniometer axis .. though that would be nice to test

Baharis commented 2 months ago

@Baharis, to elaborate, the new model only depends on the product of NaNbNc , so it might not be the most realistic..

I was confused before but now I am completely lost. Being away for the last month I probably missed some crucial information here, so maybe I'd better just refrain from unjustified comments. I'd gladly discuss this on some meeting if my input is appreciated.

dermen commented 2 months ago

@Baharis , I know you were also working on a fix for the body/face centered cells - you could use the script tst_nanoBragg_Flatt_sym.py to check that fix - I think any fix you have is still highly relevant.. the Gaussian star model is used purely for simulation, not for refinement (with gradients) .

E.g. you can try

libtbx.pthon tst_nanoBragg_Flatt_sym.py P6

and it will pass, however if you use the square RELPS, it will fail:

libtbx.pthon tst_nanoBragg_Flatt_sym.py P6 --square

We made the fix for ExaFEL by letting F_latt be a "sum of F_latts", and you can test that here:

libtbx.pthon tst_nanoBragg_Flatt_sym.py P6 --square --fix

however, as you pointed out, that will fail for I/F, e.g.

libtbx.pthon tst_nanoBragg_Flatt_sym.py F --square --fix

But the gaussian star model allows this last test to pass

libtbx.pthon tst_nanoBragg_Flatt_sym.py F

You can use --plot argument to display each of the images at the end of the script.

Baharis commented 2 months ago

@nksauter , @dermen , sorry for disappearing at the end of the meeting, it was due to some minor health issues. I believe I got a better understanding of the PR and its motivations now, I will take a second look at it this week.

dermen commented 2 months ago

I will note, I did some tests and now think the coefficient in the exponential is slightly different (I think closer to 1.75 as opposed to 1.9). Thats why the fudge is there :)