search

threeML / hawc_hal

HAWC Accelerated Likelihood - python-only framework for HAWC data analysis
BSD 3-Clause "New" or "Revised" License
12 stars 22 forks source link

Fits rois #19

Closed henrikef closed 4 years ago

henrikef commented 5 years ago

Added data_radius parameter back in for map ROI as it is needed by some of the map display functions.

henrikef commented 5 years ago

@cbrisboi or @colasri could you review this? It's a small change.

colasri commented 5 years ago

I still don't have a test environment working, so IDK. @cbrisboi ?

henrikef commented 5 years ago

After setting set_geminga_paper to xfail (probably needs fixes to astromodels, see https://github.com/threeML/astromodels/pull/101 , the linux tests pass now. MacOS still fails (ROOT fails to import).

colasri commented 5 years ago

The Geminga test used to work. When did we break it? Maybe we should slow down with the MR until the CI are back?

henrikef commented 5 years ago

It worked because the model parser in astromodels was broken and would drop some parameter links. That was fixed, see https://github.com/threeML/astromodels/pull/86#issuecomment-445961036 . Now it fails when Geminga's and Monogem's diffusion coefficients are linked, but we remove one of the sources from the model to calculate the TS. (I'd call that user error, actually).

In any case, I have a fix where astromodels will automatically unlink parameters in case of sources being removed, see https://github.com/threeML/astromodels/pull/101 . But, I need to write some unit tests and make sure it works with the current master of astromodels before merging. I did test locally and test_geminga_paper runs fine with my astromodels branch.