Update Ami analyze to input two new input parameters: JP-1742 #5548
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5548 +/- ##
==========================================
+ Coverage 71.12% 71.73% +0.60%
==========================================
Files 410 410
Lines 36339 38015 +1676
Branches 5598 6026 +428
==========================================
+ Hits 25847 27270 +1423
- Misses 8851 9053 +202
- Partials 1641 1692 +51
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report at Codecov.
|
jwst/ami/ami_analyze_step.py
Outdated
| @@ -11,6 +11,8 @@ class AmiAnalyzeStep(Step): | |||
| spec = """ | |||
| oversample = integer(default=3, min=1) # Oversampling factor | |||
| rotation = float(default=0.0) # Rotation initial guess [deg] | |||
| set_psf_offset_find_rotation = string(default='0.0 0.0') # Psf offset values to use to create the model array | |||
There was a problem hiding this comment.
Definitely agree that shorter names would be good for both of these. Perhaps just psf_offset and rotation_search?
jwst/ami/ami_analyze_step.py
Outdated
| psf_offset = self.set_psf_offset_find_rotation.split(' ') | ||
| psf_offset_find_rotation = (float(psf_offset[0]), float(psf_offset[1])) |
There was a problem hiding this comment.
Slightly more readable would be:
psf_offset_find_rotation = [float(a) for a in self.set_psf_offset_find_rotation.split()]Same below for rotsearch_parameters.
There was a problem hiding this comment.
I will make the change but it is not more readable to me. In fact I had to stare at that a while to understand what it was going
jwst/ami/ami_analyze_step.py
Outdated
| self.log.info(f'Oversampling factor = {oversample}') | ||
| self.log.info(f'Initial rotation guess = {rotate} deg') | ||
| self.log.info(f'Initial values to use for psf offset = {psf_offset_find_rotation}') | ||
| self.log.info(f'Initial values to use for rotation search {rotsearch_parameters}') |
There was a problem hiding this comment.
This logging already happens when the step is initialized. It's not as nicely formatted, but it is there. It then gets logged again, each on its own line:
2020-12-17 14:09:38,885 - stpipe.Ami3Pipeline.ami_analyze - INFO - Step ami_analyze parameters are: {'pre_hooks': [], 'post_hooks': [], 'output_file': None, 'output_dir': None, 'output_ext': '.fits', 'output_use_model': False, 'output_use_index': True, 'save_results': False, 'skip': False, 'suffix': None, 'search_output_file': True, 'input_dir': '/private/var/folders/jg/by5st33j7ps356dgb4kn8w900001n5/T/pytest-of-jdavies/pytest-30/test_niriss_ami3_run_pipeline0', 'oversample': 3, 'rotation': 1.49}
2020-12-17 14:09:38,885 - stpipe.Ami3Pipeline.ami_analyze - INFO - Oversampling factor = 3
2020-12-17 14:09:38,885 - stpipe.Ami3Pipeline.ami_analyze - INFO - Initial rotation guess = 1.49 deg
Would be nice if we could make stpipe print these to be more readable by using pprint.pformat or similar.
Fine to keep as is though I guess, as it was there originally.
Updates for ami analyze for JP-1742
The two new input parameters are 'set_psf_offset_find_rotation' and 'set_rotsearch_d'
These seems a bit long. When I was testing I tripped up the exact name of the parameter. Maybe we could use something shorter- maybe 'set_psf_offset' 'set_rotsearch' - suggestions - or just leave it as it is.
Fixes #5388 / JP-1742