-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add basic centroid file capability. #112
Conversation
Regarding the offset between the green circles and the objects in the image above: Was this with or without the sensor model, @cwwalter ? The pixel positions you are printing to the centroid files are calculated before the sensor model is applied. That might have something to do with it. |
This was without the sensor model (Just calling the drawObject version in GalSimInterpreter). I also ran with the sensor model turned on which uses GalSimSiliconInterpeter. There I saw what looked to my eye like a smaller shift if it was still there in another direction. |
This last change (along with a PR in the sims_GalSimInterface) moves the centroid information into a data structure in memory. This way it can be stored with checkpoints. The centroid files are now written out at the end of the simulation. |
in the fits directory with filenames corresponding to the fits files. The uiniqueID, the true flux and the true object position in detector coordinates are written to each file. The code to write obejct information into the filesis in sims_GalSimInterface.
files at the end of the process. This is so we can use this with the checkpointing system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good...just a few minor change requests.
python/desc/imsim/ImageSimulator.py
Outdated
@@ -34,7 +34,7 @@ class ImageSimulator: | |||
Class to manage the parallel simulation of sensors using the | |||
multiprocessing module. | |||
""" | |||
def __init__(self, instcat, psf, numRows=None, config=None, seed=267, | |||
def __init__(self, instcat, create_centroid_file, psf, numRows=None, config=None, seed=267, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to make create_centroid_file
a keyword argument with a default value of False
rather than make it a required argument, so that existing scripts won't break. I would also add it towards the end of the argument list, say after apply_sensor_model
.
python/desc/imsim/ImageSimulator.py
Outdated
@@ -125,6 +126,10 @@ def _make_gs_interpreters(self, seed, sensor_list, file_id): | |||
self.phot_params, | |||
self.obs_md) | |||
|
|||
if self.create_centroid_file is True: | |||
self.gs_interpreters[det_name].centroid_base_name = self.outdir \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It mostly doesn't matter, but it's probably better to use os.path.join
for concatenating path components.
python/desc/imsim/ImageSimulator.py
Outdated
@@ -125,6 +126,10 @@ def _make_gs_interpreters(self, seed, sensor_list, file_id): | |||
self.phot_params, | |||
self.obs_md) | |||
|
|||
if self.create_centroid_file is True: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if self.create_centroid_file is True:
-> if self.create_centroid_file:
… False instead of a required argument. Also fix up a couple of trivial PEP008 issues.
Requested changes are done in this repo. |
The code in close_centroid_files in sims_GalSimInterface was moved into write_centroid_files.
I am working on adding more pixel level truth to the simulation but thought it would be useful to first add basic 'centroid file' capability (requested in #32).
My work on adding information based on what each object looks like on the sensor is currently adding a lot of CPU run time and I need more work to optimize it. So, this initial PR uses only truth information about the objects in the centroid file. This makes it more difficult to use this to do imSim/PhoSim validation comparisons (because the PhoSim centroid values are average X and Y values on the sensor) but it is better for comparing catalog truth with the sensor since the true values are not affected by being near the edge of the sensor etc. Eventually we may want both.
This code will make centroid files corresponding to each fits file in the output directory. For example:
With a small piece of code this can be converted into a ds9 region file for overlay. This pandas code example puts a circle at each true object position with the radius scaled by the log of the number of photons for one of the centroid files:
Loading this regions file after loading the fits file results in:
To my eye, there is a small shift between the circle centers and the objects. I'm not sure if this is a plotting issue, a misunderstanding on my part of the variables I used, or some small bug in the code.
The imSim code only sets the options to turn this on and sets the file prefix in the config file. The code to open, close and write to the files is in sims_GalSimInterface. I will open a separate PR there.
Currently, this code only is in the non-sensor version of the GalSimInterpreter class. I would like some feedback from @jchiang87, @rmjarvis and @danielsf before I add much more to make sure this approach seems reasonable to people.
I started by making this look like a PhoSim centroid file since I thought this might make things easier on the people running the pipeline but in principle we could also add more information into the file.