Skip to content
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

Revised units for parameter K (ph/cm2/s) in extended source fit tutorial to match the units in the SpecFromDat file #241

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

krishnatejavedula
Copy link
Contributor

Addresses issue #194

…ial to match the units in the SpecFromDat file
Copy link

codecov bot commented Sep 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.83%. Comparing base (99fdff7) to head (2f9cfb3).

@israelmcmc
Copy link
Collaborator

Hi @krishnatejavedula, thanks for working on this. First, I verified that you are changing only this line

-    "K          = [0.00046, 0.0011, 0.0027, 4.5e-3]/u.cm/u.cm/u.s/u.keV\n",
+    "K          = [0.00046, 0.0011, 0.0027, 4.5e-3]/u.cm/u.cm/u.s\n",

the rest are just images in the notebook that are actually the same, as expected.

I think there's something still off. Looking at SpecFromDat the K should indeed be integral flux --i.e. 1/cm2/s, since we perform this normalization operation:

dataFlux = dataFlux  / np.sum(dataFlux * ewidths)

However, the evaluate() should still have units of flux density (per unit of energy) but this is not what happens. For example:

from cosipy.threeml.custom_functions import  SpecFromDat
import astropy.units as u

spec = SpecFromDat(dat="</path/to/cosipy>/docs/tutorials/spectral_fits/extended_source_fit/OPsSpectrum.dat")

K = 1/u.cm/u.cm/u.s

spec.evaluate([1]*u.MeV, K)

returns [7.35916986e-06] 1 / (cm2 s).

The issue is that when you divide by ewidths the units should also be included. As per the .dat documentation, ewidths has units of keV, the missing unit. In summary, I agree with your change in the notebook, but this should be accompanied by the appropriate change in SpecFromDat. Overall, although the result will be identical as it was before, the units and meaning of each variable will make sense.

I'm actually not sure how this will affect other code using SpecFromData. I think it's mostly the SourceInjector, so you should check. I tried to check the SourceInjector tutorial but it doesn't have units!:

spectrum = SpecFromDat(K=1/18, dat="crab_spec.dat")

I think that should be revisited as well.

@israelmcmc israelmcmc self-requested a review December 5, 2024 00:59
@israelmcmc israelmcmc self-assigned this Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants