Additional Point Source Capabilities and More #44
Conversation
smericks
commented
Jul 3, 2023
smericks
commented
- Can use apparent or absolute magnitude in light components (don't think its backwards compatible, will need review/discussion)
- add csv_path option for train_model.py
- new data generation options(option to store caustic area, option to select for quads only, option to select for high magnification of point source images)
- allow for custom pixel grid kwargs
- new sampling distributions for lens center
… newer lenstronomy
|
pandas 2.0 no longer allows DataFrame.append method: https://stackoverflow.com/questions/75956209/dataframe-object-has-no-attribute-append |
|
There was a problem hiding this comment.
Thanks for this Sydney, hope we can get your improvements in soon! I haven't gone through the Analysis and Sersic changes yet, but maybe these comments are already useful.
(We discussed offline that you would try to make the Sersic changes backwards compatible, e.g. by adding an optional argument that says whether the magnitude is specified as absolute or apparent.)
| class MagnificationError(Exception): | ||
| def __init__(self,mag_cut): | ||
|
|
||
| class FailedCriteriaError(Exception): |
There was a problem hiding this comment.
I understand you had to make this exception more generic, but if someone is interested in what criteria failed that is now hard to retrieve.
I would instead e.g. raise FailedCriteriaError("Image magnitude is X, failed the cut at Y") in the code, and to support that, either (a) remove the entire custom __init__ here or (b) let it take an extra argument which is added to the standard message.
(In the future you might want to go even further, e.g. make an exception subclass of FailedCriteriaError for each of the different errors you throw, add logic for counting how many times each comes up, etc.)
| @@ -89,6 +94,29 @@ def __init__(self,config_path,): | |||
| # Get the numerical kwargs numpix from the config | |||
| self.kwargs_numerics = self.config_module.kwargs_numerics | |||
| self.numpix = self.config_module.numpix | |||
|
|
|||
| # handle doubles_quads_only | |||
There was a problem hiding this comment.
I think you could simplify this as e.g.:
# Copy option flags from the config module to self; absence means False
flag_options = ('doubles_quads_only', 'quads_only', ...)
for option_name in flag_options:
setattr(self, option_name, getattr(self.config_module, option_name, False))| @@ -323,6 +352,27 @@ def get_metadata(self): | |||
| 'metadata.csv', | |||
| category=RuntimeWarning) | |||
| SERIALIZATIONWARNING = False | |||
|
|
|||
| # write caustic area if requested | |||
| if self.compute_caustic_area: | |||
There was a problem hiding this comment.
If self.compute_caustic_area is True, get_metadata now does a lot of computation, including re-making the LensModel. Could this be done elsewhere, maybe in _calculate_ps_metadata, where you already have the lensmodel? You can add it to the sample there, then get_metadata should just write it out automatically.
| @@ -362,6 +412,9 @@ def _calculate_ps_metadata(self,metadata,kwargs_params,point_source_model, | |||
| lens_model (lenstronomy.LensModel.lens_model.LensModel): An instance | |||
| of the lenstronomy lens model that will be used to calculate | |||
| lensing quantitities. | |||
|
|
|||
| Return: | |||
There was a problem hiding this comment.
Maybe I missed it, but I don't think this returns -1 anywhere; it either returns 0 or raises a FailedCriteriaError. If that's right you could remove this and the related logic where the return value is checked (in _draw_image_standard, _draw_image_drizzle, draw_image). And the function could just have no explicit return instead of returning 0 on success.
| @@ -414,16 +491,22 @@ def _calculate_ps_metadata(self,metadata,kwargs_params,point_source_model, | |||
| metadata[pfix+'x_image_'+str(i)] = x_image[0][i] | |||
| metadata[pfix+'y_image_'+str(i)] = y_image[0][i] | |||
| metadata[pfix+'magnification_'+str(i)] = magnifications[i] | |||
| if 'mag_pert' in sample['point_source_parameters'].keys(): | |||
There was a problem hiding this comment.
No need for .keys() here (same in a few places below)
| self.coords_dict = coords_dict | ||
| self.psf_option = psf_option | ||
|
|
||
| def hst_emp_f814w_mapping(self,coords): |
There was a problem hiding this comment.
This code looks a little scary: lots of math, hardcoded numbers, print statements, etc. It also seems pretty hubble specific. If there is no way to use an external library for some of this (e.g. maybe numpy or scipy for the interpolation?), you could move the bulk of this function to a separate function in hubble_utils.py that you can call here. And Sebastian would want you to add unit tests for this :-)
| @@ -86,15 +86,15 @@ def absolute_to_apparent(mag_absolute,z_light,cosmo, | |||
| """Converts from absolute magnitude to apparent magnitude. | |||
|
|
|||
| Args: | |||
| mag_apparent (float): The absolute magnitude | |||
| mag_absolute (float): The absolute magnitude | |||
There was a problem hiding this comment.
Thanks for fixing this very misleading docstring! 😅
| @@ -109,6 +109,34 @@ def absolute_to_apparent(mag_absolute,z_light,cosmo, | |||
|
|
|||
| return mag_apparent | |||
|
|
|||
| def apparent_to_absolute(mag_apparent,z_light,cosmo,include_k_correction=True): | |||
There was a problem hiding this comment.
You could avoid some duplication here by making one function (maybe starting with an underscore) that outputs the apparent-absolute offset and then call it from both apparent_to_absolute and absolute_to_apparent. That would make it immediately obvious there is not e.g. a minus sign error somewhere.
|
|
||
| def test_update_parameters(self): | ||
| # test passing None | ||
| self.c.update_parameters(None) | ||
| self.assertDictEqual(self.c.point_source_parameters, dict()) | ||
| # test passing an element that wasn't there yet | ||
| self.c.update_parameters({'radius':1.}) | ||
| self.c.update_parameters(None,{'radius':1.}) |
There was a problem hiding this comment.
Maybe add a comment here about what the None is doing?
| @@ -1780,7 +1780,8 @@ def test_main(self): | |||
| Analysis.train_model.main() | |||
|
|
|||
| # Cleanup the files we don't want | |||
| os.remove('test_data/fake_model.h5') | |||
| os.remove('test_data/fake_model_best.h5') | |||
There was a problem hiding this comment.
I may very well have missed it, but where do these new files get made?
Merge new changes from paltas main branch
|
Closing this large PR to make separate PRs for each feature. |
