dermen / sim_erice

1 stars 1 forks source link

Broken Bragg spots #25

Open nksauter opened 1 year ago

nksauter commented 1 year ago

@dermen look more carefully as to how to allocate granularity between mosaic domains and oversampling.

dermen commented 1 year ago

What are the commands needed to reproduce the issue: WHen I do

DIFFBRAGG_USE_CUDA=1 simtbx.sim_view

I cant seem to increase the Gaussian spectrum bandwidth beyond 2%, and I don't see broken spots at that bandwidth...

nksauter commented 1 year ago

Your difficultly may be with an unrelated bug in the spinner controls that I just reported as issue #36 . However, here is a current method for reproducing the "broken Bragg spots". Screen Shot 2023-05-16 at 11 13 46 AM Start the viewer. Click in the Domain size text box, type 8000, and hit "return". Click in the Mosaic angle text box, type 1, and hit "return". Now you should see Bragg spots that look like multiple bright pixels.

dermen commented 1 year ago

I can reproduce this - is an entry box controlling "number of mosaicity samples" an acceptable solution for the short term ? I would say we need another one for "number of spectrum energy samples"

nksauter commented 1 year ago

I was thinking those sort of hyperparameters would be in a phil scope. Less clutter for the user. Nick

On Tue, May 16, 2023 at 2:44 PM Derek Mendez @.***> wrote:

I can reproduce this - is a box controlling " number of mosaicity samples" acceptable solution for the short term ? I would say we need another one for "number of spectrum energy samples"

— Reply to this email directly, view it on GitHub https://github.com/dermen/sim_erice/issues/25#issuecomment-1550394118, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADQ24VXFSEGM7G5FVWVQCI3XGPYNTANCNFSM6AAAAAAX3UGLKM . You are receiving this because you authored the thread.Message ID: @.***>

dermen commented 1 year ago

Hey all, I wasnt sure that radians was in fact the issue - was this verified , other than visually ?

dermen commented 1 year ago

it seems like the generate_umats method actually accepts a number in degrees...

irisdyoung commented 1 year ago

@dermen I reverted the (apparently wrong) conversion to radians. We have a hyperparam now for number of mosaic domains as well as one for oversampling. Can you try those out and see if adjusting these can smooth the broken spots?

It's not yet clear to me which of these cases we're in:

  1. math/science/calcs are factually wrong
  2. simulation isn't visually representative of reality due to limitations of sampling
  3. simulation isn't visually representative of reality due to poor choice of default params

Still need help sorting this one out.

dermen commented 1 year ago

TL;DR I have concluded this issue partly arises from mis-using nanobragg (at least the given reproducer). Its impractical to allow users to simulate huge mosaic blocks. I don't have a specific solution, but I have some design choices below that make most sense based on my use of nanoBragg (and a desire to add background to the images).. But as it stands, the quickest way to "solve" this issue is adding an oversample and number_of_energy_channels field for users , and beyond this we also should impose some threshold parameters (e.g. maximum domain size for a given oversample, maximum bandwidth for a given "number of energy channels").

===================================

This code:

def on_update_domain_size(self, skip_gen_image_data=False):
        """given a target domain size (one side length), determine number of cells to use along each axis"""
        side_length = self.params_num.domain_size.get_value()
        a, b, c, _, _, _ = self.scaled_ucell
        self.ncells = []
        for side in (a, b, c):
            self.ncells.append(max(int(round(side_length/side)), 1))
        if not skip_gen_image_data:
            self._generate_image_data()

updates Ncells_abc when domain size is increased.. However this is dangerous, as we know nanoBragg can only handle Ncells_abc up to certain values (and small values). Simulating e.g. a 200x200x200 mosaic block will indeed require a large amount of oversample.

When I start the GUI, self.ncells is 13x26x26 (maximum pixel intensity = 9.65e-6), however after toggling the mosaic domain size to 9000 (from 1000), ncells becomes 114x114x234 (maximum pixel intensity=0.017). Note, these pixel intensities are way too small.. If I toggle "Use structure factors" , then things get better (0.33 max intensity for 1000 domain size, 421 max intensity for 9000 domain size), but that's an aside for this issue.

I propose two solutions that will help fix this "issue" (the "issue" arises from mis-using nanoBragg as its currently written, and we need to guard against this).

a) allow the user to enter 3 numbers (Na, Nb, Nc). This is important, because the aspect ratios of the mosaic block influence the positions of the spots (think spherical block versus skinny ellipsoidal block). So users will provide Na,Nb,Nc (we will compute a "mosaic block size" automatically, and display it to the user). Then, user will provide a "crystal size " parameter, which will compute a volume scale factor to bring the scattering up to physical units (so we can add a physical background)..

b) user provides two "size" parameters, one for block size (as it is now), and another for crystal size. the former will have a restriction which we pre-compute ahead of time (e.g. max block size determined from a 90x90x90 block or something dependent on the unit cell). Similar to part a), the crystal size will be used to compute a scale factor to bring the scattering up to physical units..

Otherwise, the user will never know when they are entering the out-of-scope region (too large of a mosaic block).

Now we can always increase oversample, but things can also easily get out-of-hand and impractical. We just need to make this clear to the user in order to fix this issue..

Also, as an aside, we need to scale the self.SIM_noSF model to have the same "structure factor intensity" as the self.SIM model.. This will fix the issue above that default SIM has really small (1e-6) pixel intensities..