Thick Target (and thermal) Update - #291
Open
KriSun95 wants to merge 10 commits into
Open
Conversation
…d_equivalent_units()
…e. Similar to other functions.
samaloney
reviewed
Aug 6, 2026
|
|
||
| __all__ = ["ThickTarget", "ThinTarget"] | ||
|
|
||
| FLOAT_EPSILON_FOR_POWER_LAW = 1 + FLOAT_EPSILON * 1e30 |
Member
There was a problem hiding this comment.
Where does the FLOAT_EPSILON * 1e30 come from, trail and error or something else?
Comment on lines
+16
to
+18
| scaled_thick_eflux_units = u.def_unit("scaled_thick_eflux_units", 1e35 * (u.electron * u.s**-1)) | ||
| scaled_thin_eflux_units = u.def_unit("scaled_thin_eflux_units", 1e55 * (u.electron * u.cm ** (-2) * u.s**-1)) | ||
| scaled_em_units = u.def_unit("scaled_em_units", 1e49 * (u.cm ** (-3))) |
Member
There was a problem hiding this comment.
On the naming these aren't scaled they are the thick_eflux units etc, no?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR got a bit away from me but it was all in the investigation of trying to figure out why the Thick target model would return NaNs sometimes.
So, what I've done is:
norm_thick_target_eflux_units,norm_thin_target_eflux_units, andnorm_thermal_emission_measure_units.5e49 cm^-3, if we like but internally the value will be converted to the scaled version of5 new_unitswherenew_untisis1e49 cm^-3. This is unit aware and so conversions can easily happen that the code knows about rather than it being in some places in the doc string that5 cm^-3really means5e49 cm^-3.5e49 cm^-3and5 new_unitswork the same and as intended.RunTimeWarningsas they should.<<when the model evaluates. This means we don't need to haveif/elsestatements checking all the input units then assuming the units if they're not there.The above were fixes I made to keep investigating the weird NaN behaviour I've seen before, removing cases where this has happened. I then ran a fit, producing the plot below (data=thermal+thick source, model=thermal+thick model, thick=just the thick target model contribution):
From here, the thick target is returning one bin around 20 keV with a NaN.
Investigating this further, I think there are two important points:
energy_edges = np.arange(2, 15, 0.1) << u.keVinstead does not return a NaN in that bin.Still to do:
Following the Astropy Parameter docs, add a getter/setter to the flux and emission measure parameters (those that are scaled) so that if the parameter is edited outside initialising the model or calling evaluate (e.g., just doing something like.emission_measure = 3e50<<u.cm**-3) the units will be converted to the new scaled units for use internally to the model.The code I used to make the plot is here:
With output (the arrays being printed are those that contain a NaN during the parameter search during fitting with the parameters producing the array printed on the line before):