Evolution of the spatial fitting to astropy model and preparation of the future spectral fitting model#55
Conversation
gabemery
left a comment
gabemery
left a comment
There was a problem hiding this comment.
Impressive work. It looks a bit less user friendly for users to provide functions, but if it is well documented it should not be a large issue.
gabemery
left a comment
gabemery
left a comment
There was a problem hiding this comment.
Looking in depth to the fitting section, it seems like this implementation makes barely any use of the astropy implementation of models and, especially, fitters. In the current state, I would remove the dependency to Fitter.
|
|
||
| # build list of free parameters | ||
| all_params = list(model_copy.param_names) | ||
| free_params = [p for p in all_params if (p not in tied) or (not tied[p])] |
There was a problem hiding this comment.
It could probably be done a bit easier with sets. But it works.
There was a problem hiding this comment.
I'm not very familiar with sets, feel free to propose.
| free_params = [p for p in all_params if (p not in tied) or (not tied[p]) and (not model_init.fixed.get(p, False))] | ||
|
|
||
| # Apply correction | ||
| for p in free_params: |
There was a problem hiding this comment.
We should support normalising and fixing the normalisation. So not removing fixed parameters.
In that case, and if you also remove the boolean check on the tied, these lines could be replaced by :
for p in set(model_init.param_names) - set(tied.keys())) :
exemple code :
test={'a':0, 'b':0}
set(['a','c']) - set(test.keys())
-> {'c'}
There was a problem hiding this comment.
If you apply normalisation on a tied parameters it will be not be considered, as it will be replaced at the start of the fit. Instead I added a warning when a parameters in the list list_name_normalisation_parameter is not found in the indep_params list.
gabemery
left a comment
gabemery
left a comment
There was a problem hiding this comment.
Couple comments on the latest changes.
dbfd91e to
e23c8d7
Compare
mdebony
left a comment
mdebony
left a comment
There was a problem hiding this comment.
I've left a few comments. The main one is than we need to set some reference values and models for tests.
There was a problem hiding this comment.
Would be good to have check on the values in the test.
| # test having one bad paramenter in the list to normalise | ||
| list_name_normalisation_parameter=['amplitude', 'bad_param']) | ||
| background_model = bkg_maker.create_model(observations=self.obs_collection_pks_2155) | ||
| assert type(background_model) is Background3D |
There was a problem hiding this comment.
We need to add a reference as for the other test
There was a problem hiding this comment.
I was thinking that this is something you are the best placed to do since you did it for the other cases.
| model_to_fit=model, | ||
| list_name_normalisation_parameter=['amplitude',]) | ||
| background_model = bkg_maker.create_model(observations=self.obs_collection_pks_2155) | ||
| assert type(background_model) is Background3D |
There was a problem hiding this comment.
We need to add reference data and check on it.
| return (powerlaw_inv_super_exp_cutoff_exp_cutoff(e, amplitude, index, eref, b, ecl, ech) * | ||
| gaussian2d_bilinear_gradient(x, y, 1.0, x_mean, y_mean, x_stddev, y_stddev, theta, x_gradient, y_gradient)) | ||
|
|
||
| class PowerLawCutOffsEnergy(CustomModels): |
There was a problem hiding this comment.
I think the name could be confused with the standard power law exponential cut off in the field. Not sure exactly how to name it.
There was a problem hiding this comment.
Yes, without making it very long I had no better ideas. Or give it a physics related name
… Added new models.
641538f to
05d90bf
Compare
…ort for positionnal arguments in fitting class.
Integration in the fitting procedure
gabemery
left a comment
gabemery
left a comment
There was a problem hiding this comment.
The PR is ready for merging.
2 participants
I have change the organization of the fitting procedure that now is in it's own class. It's to prepare for adding spectral fit procedure.
I have also change the fitting procedure to use astropy models for describing model to fit instead of our own set of functions. This point need a bit more validation.
@gabemery contributions:
Changed the spatial fit class to handle one to three dimensions (assumed energy , spatial 2D in this order if 3D).
Added a new class to define models from a function and used it to implement some models.
Added tests for the new functionnalities.