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

Unable to fit with load_multi_arf/load_multi_rmf: startup() takes 1 positional argument but 2 were given #717

Closed
DougBurke opened this issue Dec 15, 2019 · 1 comment

Comments

@DougBurke
Copy link
Contributor

Summary

Noted by SDS, Sherpa 4.12 (CIAO 4.12) has the following error:

sherpa In [1]: load_pha(1, "460_leg_m1_bin10.pha")                                                                                                               
WARNING: systematic errors were not found in file '460_leg_m1_bin10.pha'
statistical errors were found in file '460_leg_m1_bin10.pha' 
but not used; to use them, re-read with use_errors=True
read background_up into a dataset from file 460_leg_m1_bin10.pha
read background_down into a dataset from file 460_leg_m1_bin10.pha

sherpa In [2]: load_multi_arfs(1, ["460_LEG_-1.garf","460_LEG_-2.garf","460_LEG_-3.garf"], [1,2,3]) 

sherpa In [3]: load_multi_rmfs(1, ["460_leg_-1.grmf","460_leg_-2.grmf","460_leg_-3.grmf"], [1,2,3]) 

sherpa In [4]: mdl = xswabs.abs1 * xsbknpower.bpow1 

sherpa In [5]: set_model(1, mdl)                                                                                            

sherpa In [6]: bpow1.norm = 0.05                                                                                            

sherpa In [7]: plot_fit(ylog=True)                                                                                          

The plot works - i.e. you can evaluate the model - but when you come to try to fit it (the sys.tracebacklimit setting is to make sure we get a reasonable traceback here) the fit fails

sherpa In [8]: import sys                                                                                                   

sherpa In [9]: sys.tracebacklimit = 1                                                                                       

sherpa In [10]: fit()                                                                                                       
WARNING: data set 1 has associated backgrounds, but they have not been subtracted, nor have background models been set
  File "/home/dburke/anaconda3/envs/ciao412/lib/python3.7/site-packages/IPython/core/interactiveshell.py", line 3319, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
TypeError: startup() takes 1 positional argument but 2 were given

Workaround

This can be worked around by monkey-patching the startup method of the problematic class before any instances have been created:

# patch the startup method of the MultiResponseSumModel *before* any
# instances have been created
#
import numpy
from sherpa.models.model import CompositeModel
from sherpa.astro.instrument import MultiResponseSumModel

def startup_monkey(self, cache):
    pha = self.pha
    if numpy.iterable(pha.mask):
        pha.notice_response(True)
    self.channel = pha.get_noticed_channels()
    self.mask = pha.get_mask()
    self._get_noticed_energy_list()
    CompositeModel.startup(self, cache)

MultiResponseSumModel.startup = startup_monkey

If you have this in a file (e.g. patchit.py) then you can

sherpa In [1]: %run -i patchit.py

and then you can fit data with multiple ARF/RMF

Details

This is similar to the issue that lead to #701 - a startup method needs to know about the cache argument. Should we make the default cache routine have *args / **kwargs so that this doesn't happen (there are plenty of other startup methods in classes which take no arguments)?

The fix for this particular case is easy, but we have no tests for load_multi_arfs / load_multi_rmfs which is going to take longer to put together.

DougBurke added a commit to DougBurke/sherpa that referenced this issue Apr 4, 2020
The test added in sherpa#728 to fix sherpa#717 has been changed from using the
xsapec model to powlaw1d, and commentary added on what system was
used to calculate the test values. This has the advantage of
allowing the test to run when XSPEC is not available, but the main
benefit is that the model used (power-law) is much simpler, and so
less-likely to change over time (whereas the fit parameters for
xsapec changed significantly in XSPEC 12.11.0 compared to XSPEC
12.10.1 due to internal changes in the APEC/ATOMDB handling in
XSPEC).
DougBurke added a commit to DougBurke/sherpa that referenced this issue Apr 14, 2020
The test added in sherpa#728 to fix sherpa#717 has been changed from using the
xsapec model to powlaw1d, and commentary added on what system was
used to calculate the test values. This has the advantage of
allowing the test to run when XSPEC is not available, but the main
benefit is that the model used (power-law) is much simpler, and so
less-likely to change over time (whereas the fit parameters for
xsapec changed significantly in XSPEC 12.11.0 compared to XSPEC
12.10.1 due to internal changes in the APEC/ATOMDB handling in
XSPEC).
@DougBurke DougBurke added this to the 4.12.1 milestone Jun 17, 2020
@DougBurke
Copy link
Contributor Author

Closing as fixed in #728

DougBurke added a commit to DougBurke/sherpa that referenced this issue Jul 21, 2020
The test added in sherpa#728 to fix sherpa#717 has been changed from using the
xsapec model to powlaw1d, and commentary added on what system was
used to calculate the test values. This has the advantage of
allowing the test to run when XSPEC is not available, but the main
benefit is that the model used (power-law) is much simpler, and so
less-likely to change over time (whereas the fit parameters for
xsapec changed significantly in XSPEC 12.11.0 compared to XSPEC
12.10.1 due to internal changes in the APEC/ATOMDB handling in
XSPEC).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant