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

Gauss star gonio #985

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Gauss star gonio #985

wants to merge 13 commits into from

Conversation

dermen
Copy link
Contributor

@dermen dermen commented Apr 21, 2024

This includes two big additions to diffBragg and modeling

  • the use of a perfectly round RELP called gauss_star that eliminates issues where F_latt varies with symmetry operations
  • full support of goniometer rotation in diffBragg

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
Copy link
Contributor

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
Copy link
Contributor

Baharis commented Apr 21, 2024

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
Copy link
Contributor Author

dermen commented Apr 22, 2024

@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..

@dermen dermen requested a review from Baharis April 22, 2024 00:59
@Baharis
Copy link
Contributor

Baharis commented Apr 22, 2024

@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
Copy link
Contributor Author

dermen commented Apr 22, 2024

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

@dermen
Copy link
Contributor Author

dermen commented Apr 22, 2024

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
Copy link
Contributor

Baharis commented Apr 22, 2024

@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
Copy link
Contributor Author

dermen commented Apr 22, 2024

@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
Copy link
Contributor

Baharis commented Apr 24, 2024

@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
Copy link
Contributor Author

dermen commented Apr 29, 2024

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 :)

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.

3 participants