-
Notifications
You must be signed in to change notification settings - Fork 12
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
Common base class for extensibility and reduced boilerplate #40
base: main
Are you sure you want to change the base?
Conversation
I experimented with a wrapper to convert Sources to lens light classes here: JelleAalbers@9d1cee2 (+ a few bugfixes in later commits). Let me know if you'd like something like that here (or in a future PR), I'm on the fence as to whether it actually made things clearer or not. |
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.
Overall an excellent change that refactors a lot of repeated lines of code and makes it easier to build extensions moving forward. Thank you! I included a few nitpick comments. Other than those, please merge in the new main branch to confirm that all tests are still passing with these changes.
self.cosmo = get_cosmology(cosmology_parameters) | ||
# Historical oversight: cosmology parameters as first arg... | ||
init_kwargs = ('cosmology_parameters', 'source_parameters') | ||
main_param_dict_name = 'source_parameters' |
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.
Why did you put source_parameters as the second init_kwargs here and then manually set main_param_dict_names?
"""Updated the parameters | ||
|
||
Args: | ||
Dictionary for each argument named in _init_kwargs |
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 would be good to describe the use of _first_time here even if it is a "private" input to the function.
|
||
@classmethod | ||
def init_from_sample(cls, sample): | ||
return cls(**cls._sample_to_kwargs(sample)) |
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.
Please include docstring.
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.
This is a nitpick, but it would be nice to have an explicit unit test for this function, update_from_sample, and _sample_to_kwargs. They do get called by other unit tests, but it seems like good form to have unit testing of what will now be the most core class in the package.
def init_from_sample(cls, sample): | ||
return cls(**cls._sample_to_kwargs(sample)) | ||
|
||
def update_from_sample(self, sample): |
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.
Please include docstring.
self.update_parameters(**self._sample_to_kwargs(sample)) | ||
|
||
@classmethod | ||
def _sample_to_kwargs(cls, sample): |
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.
Please include docstring.
|
||
class BaseComponent: | ||
"""Base class for all paltas models (e.g. SourceBase) """ | ||
|
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.
Add a description of the args (in reference to the init_kwargs)
kwargs_source=[] | ||
) | ||
|
||
def add_lenses(self, models, model_kwargs, redshifts): |
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.
Please add a docstring.
self.kwargs_params['kwargs_lens'] += model_kwargs | ||
self.kwargs_model['lens_redshift_list']+= redshifts | ||
|
||
def add_sources(self, models, model_kwargs, redshifts): |
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.
Please add a docstring.
self.kwargs_params['kwargs_source'] += model_kwargs | ||
self.kwargs_model['source_redshift_list'] += redshifts | ||
|
||
def add_point_sources(self, models, model_kwargs): |
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.
Please add a docstring.
self.kwargs_model['point_source_model_list'] += models | ||
self.kwargs_params['kwargs_ps'] += model_kwargs | ||
|
||
def add_lens_light(self, models, model_kwargs, redshifts=None): |
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.
Please add a docstring.
8ea5bc2
to
e1c1958
Compare
Hi Jelle, what is the status of this pull request? Are you waiting for another review on the changes? |
This introduces two changes:
SourceBase
,LOSBase
, etc.) subclasses of a newBaseComponent
class. This base class takes care of the common init, update, and parameter checking that was previously duplicated across components.ConfigModel.get_lenstronomy_models_kwargs
to remove the mostly-duplicated code for initializing and drawing from the different models. In particular, paltas classes now have adraw
method that calls methods likeadd_lenses
oradd_sources
from a commonLenstronomyInputs
object that is passed around. Besides cleaning things up, this will allow us to add features like painting galaxies onto subhalos.I put the new base class in a top-level
core.py
. If you like that, maybe we can move theConfigHandler
stuff there too. We could also squirrel away the base class in an obscure utils file.One remaining eyesore is the lens light handling. Perhaps we could implement lens light by adding a source model at the lens redshift, instead of collecting separate buckets of lenstronomy arguments? This would require more changes in the test suite though.
The tests will fail until #38 (or at least 94bfd3d) is merged and we rebase.