Adds Astropy wrapper to the thermal model #189
Conversation
Created an Astropy model class wrapper for the thermal model which seems to be working with correct unit handling, however, need to carry out some tests and add in smilar wrappers for contimuum and line models.
|
Looks like it nearly ready, nice work. What's up with the bumps in the albedo model? |
Unsure to be honest, am looking into it rn, I don't see it when evaluating the albedo model with a simulated spectrum, as in the example code from the albedo model. |
|
the bumps look like they might be due to an interpolation issue---for things like mass attenuation coefficients, they need to be interpolated in (log(x), log(y)) space to keep the resulting log-log plots smooth. is the albedo data interpolated in that way? |
|
@natsat0919 and I realised that the green matrices have a resolution of 1keV, which is whats causing the strange patterns. This issue is present in the legacy code, however it wouldn't be apparent when using STIX data. To solve this, we should run off some higher resolution tables. @samaloney would you be able to host them? |
| relative_abundances=None, | ||
| **kwargs): | ||
|
|
||
| self.energy_edges = kwargs.pop("energy_edges") |
There was a problem hiding this comment.
should energy_edges be an instance variable? i thought they got passed to evaluate
There was a problem hiding this comment.
good point, I have changed this now, will push the changes soon along with some others.
Six key elemental abundances have been added as free parameters and energy_edges removed as an instance variable, now it is passed to evaluate.
Reinstates _sanitize_inputs method to handle units correctly for astropy fitting.
Corrects abundances for caluclation and allows users to specify a different table than the default and updates the default abundances accordingly.
Adds Al abundance as a parameter
| abundances[20] = ca | ||
| abundances[26] = fe | ||
|
|
||
| # if relative_abundances: |
There was a problem hiding this comment.
Might be worth to keep this code for relative abundances? Not sure what others think
There was a problem hiding this comment.
It could cause a slight issue with the inclusion of some of the abundances as fittable parameters, but I'm sure we could get round that if this is a desired feature. Would it be useful for people seeing as the abundances of the seven fittable elements can now be set and tied relatively by users?
|
I want to start on the thick target model also, should I do that in this PR, or open a separate one using this same branch? It is important that I have the ThermalEmission class in the same branch as I want to test the model combination. |
I would suggest to start a new branch. Both models are quite a lot of work with a lot of code so it would be best to keep them separate |
Adds Astropy wrappers to line_emission and continuum_emission, LineEmission and ContinuumEmission
Adds an astropy model class wrapper to the thermal model
This PR changes the format of the thermal model by adding an astropy class wrapper, ThermalEmission.
Currently the model can be evaluated both alone and when combined with the albedo model. Below is a plot showing the results of evaluating the model both alone and with the albedo model:
The model can be fit with the standard astropy fitting packages as a standalone.
The following needs to be changed before merging: