#722 #869
#722 #869
Conversation
|
@craiglagegit I fixed up the merge. But you'll need to do a hard reset now rather than a regular git pull: Also, I got rid of the min_charge change you made, since we don't actually need that in the c++ layer anymore. I had rearranged the part of the code that used it, so it's not actually helpful anymore. I still need to fix the directory specification to not use your local path, but I'll do that next. And make sure there is a way to access the photon array from a |
|
OK, I added a option to |
…dom number realization.
galsim/sensor.py
Outdated
| pixel distortions, and the *_Vertices.dat file which carries the | ||
| distorted pixel information. [default: 'lsst_itl'] | ||
| @param strength Set the strength of the brighter-fatter effect relative to the | ||
| amount in specified by the Poisson simulation results. [default: 1] |
There was a problem hiding this comment.
I switched the num_elec parameter to a strength parameter, since you have the reference number of electrons stored in the config file now, so the only purpose left for this was to change the strength of the effect. I think it will be easier for users to just enter strength=2 if they want to double the strength of the effect relative to the simulator results. This is equivalent to setting num_elec = config['CollectedCharge_0_0'] / 2 in the old scheme.
src/Silicon.cpp
Outdated
|
|
||
| // Now we add in a displacement due to diffusion | ||
| if (_diffStep != 0.) { | ||
| double diffStep = std::max(0.0, diffStep_pixel_z * (zconv - _pixelSize)); |
There was a problem hiding this comment.
I'm not sure about this line. This used to be zconv - 10.0, and you switched the 10.0 to _pixelSize. But why would you be subtracting the pixel size here? I thought the 10.0 was just some kind of buffer to keep it from diffusing too much if zconv was close to 0.
Like maybe once it got 10 microns from the surface, the electric fields were strong enough that it doesn't really diffuse anymore. Or something like that anyway. But I don't see how the pixel size would be relevant.
So could you double check the math on this line? Maybe the 10.0 here should be a different input parameter? (Perhaps one that we don't have named yet?)
Removed duplicate lsst_itl file. Updated bf_plots.doc and bf_plots_3Mar17.py
|
OK, I corrected the overzealous search/replace error you pointed out in the diff step calculation, removed the duplicate sensor data in devel, and updated the bf_plots stuff in devel/brighter-fatter so it writes out the photon_file. FYI, I added a conversion file in Poisson_CCD to convert the fits file to a text file that I can read into Poisson_CCD. As far as I am concerned we are good to go. I will keep working to try to get to the bottom of the BF slope discrepancy, but after all of these changes it still looks the same as it did after the SLAC DESC hack day. |
|
Thanks Craig. Looks good to me. I think @rmandelb wanted to review it too, so we can wait for her before merging. |
|
@rmjarvis - I am not sure that you should wait for me. I was selected to sit on a jury on Wednesday and the case is still not over, so I've effectively been unable to work for 3 days with at least one more to go. When it's over, I will have to go do a bunch of other time-critical things that I was supposed to do in the past few days. So at best, I could get to this in a week. You might want to just go ahead... |
|
OK. Let's plan to merge this on Wednesday evening then to give a few more days for anyone else who might want to take a look. I don't think there's a big rush, since there are still a few other issues we're planning to get into v1.5. |
galsim/gsobject.py
Outdated
| @@ -1238,10 +1239,17 @@ def drawImage(self, image=None, nx=None, ny=None, bounds=None, scale=None, wcs=N | |||
| @param surface_ops A list of operators that can modify the photon array that will be | |||
| applied in order before accumulating the photons on the sensor. | |||
| [default: ()] | |||
| @param sensor An optional Sensor instance, which will be used to accumulate the | |||
There was a problem hiding this comment.
I think it would be worth mentioning in the drawImage docstring (above the param section somewhere) something about how the Sensor mechanics work when using FFT methods. From the code I gather that the procedure is to first draw an image, sample photons from that image as if it were a probability distribution function, and then pass those photons to the sensor code. (Did I get that right?)
Also, are there any combinations of sensor != None and method != 'phot' that we should warn on? I presume only one of sensor=SiliconSensor, method='fft' and sensor=SiliconSensor, method='no_pixel' would be reasonable?
There was a problem hiding this comment.
You're correct about the mechanics of using the sensor with FFT methods. I think they are accurate in the limit of the intermediate image having infinitely small pixels, regardless of no_pixel or not.
But your comment reminded me that right now the intermediate image has the same size pixels as the final image. (Which means it's not quite correct for either 'no_pixel' or 'fft' currently.) I'd meant to add a parameter that would let the user specify how much smaller the intermediate image pixels should be. Probably with a default of 2 or 3 maybe.
And I'll add a comment about all of that in the doc string.
There was a problem hiding this comment.
Josh, I finally got around to doing this. I added Image methods bin and subsample which are kind of converse operations to bin pixels together or split them up into subpixels. And now using fft drawing methods with a sensor defaults to subsampling 3x3. I also added a bunch of text to the drawImage doc string about this and the other new sensor stuff. Please take a look the recent few commits.
There was a problem hiding this comment.
Thanks Mike, Looks good to me. I fixed some typos and an killed an unused variable. Assuming that gets through travis, I think this is good to merge.
tests/test_sensor.py
Outdated
| rng2 = galsim.BaseDeviate(5678) | ||
| rng3 = galsim.BaseDeviate(5678) | ||
|
|
||
| # TODO: This file needs a better location (and probably name). |
|
Great. Thanks, Josh! |
This version contains the code to implement the brighter-fatter effect, with each photon having a specific wavelength and angles as drawn from a distribution. I have tested it and believe it is working OK. The file examples/brighter-fatter/GalSim_17Feb17.pdf shows the results of the B-F slopes. The slopes are larger than the measurements, and this discrepancy is not understood at this time. The magnitude is easily adjusted, and I did this last October, but chose not to do that this time. However, I think we should still merge this into the master branch while we study the reason for the larger slopes. The tests in tests/test_sensor.py also seem to run OK.