PSF config handling and checkpoint filename fixes for Run2.1i #211
Conversation
… into u/jchiang/read_config_fix
…esses in a multiprocessing.Pool
|
@villarrealas There are a couple changes to the checkpoint filenames to note:
|
| @@ -257,8 +257,8 @@ def run(self, processes=1, wait_time=None, node_id=0): | |||
| pool = multiprocessing.Pool(processes=processes) | |||
| receivers = [] | |||
| if wait_time is not None: | |||
| results.append(pool.apply_async(process_monitor, (), | |||
| dict(wait_time=wait_time))) | |||
| results.append(pool.apply_async(TracebackDecorator(process_monitor), | |||
There was a problem hiding this comment.
I know this is wrapping a function just like a decorator with @ would do. Could you also use it that way? If not, I wonder if it would be better to call it something like TracebackWrapper?
There was a problem hiding this comment.
"Decorator" has an established meaning as a standard design pattern, which this class satisfies. "Wrapper" also has an established design pattern meaning, which does not fit this class, so I think the current class name is the right one.
| added_keywords: dict [None] | ||
| Python dict of key/value pairs to add to the primary HDU. | ||
| These will be set just before writing the FITS file, so | ||
| they will override any existing keywords. |
There was a problem hiding this comment.
maybe add:
'they will override any existing keywords or be added to the header list if they don't exist.'
There was a problem hiding this comment.
The name of the option indicates that the keywords will be added and the first sentence of the description also says that they will be added. I think it's probably ok as-is.
| visit = self.obs_md.OpsimMetaData['obshistID'] | ||
| CHECKPOINT_SUMMARY \ | ||
| = CheckpointSummary(db_file='ckpt_{}_{}.sqlite3'.format(visit, node_id)) | ||
| db_file = 'ckpt_{}_{}.sqlite3'.format(self.file_id, node_id) |
There was a problem hiding this comment.
What's the rational for using the file_id command line parameter here instead of the visit_id as before (and same question for the the checkpoint)?
There was a problem hiding this comment.
visit_id was too restrictive. If I want to put additional information in the checkpoint summary file name (e.g., the filter), then file_id is the better way to do it.
|
|
||
| if not cp.read(config_file): | ||
| raise FileNotFoundError("Config file {} not found".format(config_file)) | ||
|
|
There was a problem hiding this comment.
GitHub isn't letting me comment on the line since it hasn't changed, but below here (on line 776) you are still calling read_config (right after the function definition). But you are now doing this on line 57 in bin/imSim.py.
Will this wipe out any parameters that were changed programmatically since then?
There was a problem hiding this comment.
The read_config here picks up the defaults so that I can pop into ipython and not have to call read_config explicitly if I want to do something simple, like read the observing parameters from an instance catalog or construct an obs_md object from it. The read_config call at https://github.com/LSSTDESC/imSim/blob/u/jchiang/read_config_fix/bin.src/imsim.py#L57 ensures that any user provided config is read in and will be used when the code is executed.
| @@ -50,4 +50,4 @@ sort_magnorm = True | |||
| # FWHM in arcsec of the Gaussian to convolve with the baseline | |||
| # AtmosphericPSF + OptWF PSF to account for additional instrumental | |||
| # effects. | |||
| gaussianFWHM = 0.3 | |||
| gaussianFWHM = 0.4 | |||
There was a problem hiding this comment.
Do we have the results in a histogram from the multiple visits you ran of ellipticity etc from Javier yet, to know if this is the value we want to use?
There was a problem hiding this comment.
We had an extensive discussion on this at XWG computing group session on Wednesday of the DESC meeting, and everyone agreed that this was the value we should use.
No description provided.