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

save_all for 2D dataset does not save set_psf() #1873

Closed
kglotfelty opened this issue Sep 6, 2023 · 2 comments · Fixed by #1874
Closed

save_all for 2D dataset does not save set_psf() #1873

kglotfelty opened this issue Sep 6, 2023 · 2 comments · Fixed by #1874
Labels

Comments

@kglotfelty
Copy link
Collaborator

I noticed from a CIAO helpdesk ticket that the sherpa save_all output does not save the equivalent of set_psf.

For example:

load_data('a.img')
load_psf('p0', 'a.psf')
set_source(gauss2d.g0)
set_psf(p0)

save_all('my.script')

and look at the my.script output, it loads the PSF,

...
load_psf("p0", "a.psf")
p0.size = (101, 101)

p0.center = (50, 50)

create_model_component("gauss2d", "g0")
...
set_source(1, gauss2d.g0)

but it never runs set_psf. When I run the script (well, I just cut-n-pasted it) then the PSF is not actually used.

Tested Linux RH8 CIAO 4.14, 4.15 and ciaox (Sherpa 4.15.1+196.g15479eb1)


dummy data

echo 50 50 > a
aconvolve "a[bin col1=-0.5:100.5:1,col2=-0.5:100.5:1]" a.psf "lib:gaus(2,5,5,5,5)" norm=none cl+
python << EOM
from sherpa.astro.ui import *;
load_data("a.psf")
load_table_model("pp", "a.psf")
set_source(pp)
fake()
save_data("a.img", ascii=False, clobber=True)
EOM
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 6, 2023
The problem is that we do not include the "set_psf" call in the
ASCII serialization. Since it's not obvious what the text will be
we can't check the output serialization, but we can at least
check that the PSF has not been applied when we restore the
saved file.
@DougBurke
Copy link
Contributor

So, it's supposed to add the set_psf line - see

elif typename == "psfmodel":
cmd = 'load_psf("%s", "%s")' % (mod._name, mod.kernel.name)
_output(cmd, fh)
try:
psfmod = state.get_psf(id)
cmd_id = _id_to_str(id)
cmd = "set_psf(%s, %s)" % (cmd_id, psfmod._name)
_output(cmd, fh)
except:
pass

but it isn't. One of the problems with a big "try/except` block - it's easy to not notice it's skipping for some other issue....

@DougBurke
Copy link
Contributor

Do, it's because id isn't defined here, so it's using the Python id symbol, and causing no-end of problems.

Doh.

DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 6, 2023
This was a logical error in the existing code, coupled with
incomplete testing.

The fix is simple: we just need to loop through the session's
_psf dictionary and use the key,value as the arguments to
set_psf call(s).
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 7, 2023
The problem is that we do not include the "set_psf" call in the
ASCII serialization. Since it's not obvious what the text will be
we can't check the output serialization, but we can at least
check that the PSF has not been applied when we restore the
saved file.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 7, 2023
This was a logical error in the existing code, coupled with
incomplete testing.

The fix is simple: we just need to loop through the session's
_psf dictionary and use the key,value as the arguments to
set_psf call(s).

There is still something weird going on with the warning messages
for this test. I don't understand but have got it to a point
where it works for me.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 7, 2023
This was a logical error in the existing code, coupled with
incomplete testing.

The fix is simple: we just need to loop through the session's
_psf dictionary and use the key,value as the arguments to
set_psf call(s).

There is still something weird going on with the warning messages
for this test. I don't understand but have got it to a point
where it works for me.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 8, 2023
The problem is that we do not include the "set_psf" call in the
ASCII serialization. Since it's not obvious what the text will be
we can't check the output serialization, but we can at least
check that the PSF has not been applied when we restore the
saved file.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 8, 2023
This was a logical error in the existing code, coupled with
incomplete testing.

The fix is simple: we just need to loop through the session's
_psf dictionary and use the key,value as the arguments to
set_psf call(s).

There is still something weird going on with the warning messages
for this test. I don't understand but have got it to a point
where it works for me.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 9, 2023
The problem is that we do not include the "set_psf" call in the
ASCII serialization. Since it's not obvious what the text will be
we can't check the output serialization, but we can at least
check that the PSF has not been applied when we restore the
saved file.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 9, 2023
This was a logical error in the existing code, coupled with
incomplete testing.

The fix is simple: we just need to loop through the session's
_psf dictionary and use the key,value as the arguments to
set_psf call(s).

There is still something weird going on with the warning messages
for this test. I don't understand but have got it to a point
where it works for me.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 10, 2023
The problem is that we do not include the "set_psf" call in the
ASCII serialization. Since it's not obvious what the text will be
we can't check the output serialization, but we can at least
check that the PSF has not been applied when we restore the
saved file.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 10, 2023
This was a logical error in the existing code, coupled with
incomplete testing.

The fix is simple: we just need to loop through the session's
_psf dictionary and use the key,value as the arguments to
set_psf call(s).

There is still something weird going on with the warning messages
for this test. I don't understand but have got it to a point
where it works for me.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 11, 2023
The problem is that we do not include the "set_psf" call in the
ASCII serialization. Since it's not obvious what the text will be
we can't check the output serialization, but we can at least
check that the PSF has not been applied when we restore the
saved file.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 11, 2023
This was a logical error in the existing code, coupled with
incomplete testing.

The fix is simple: we just need to loop through the session's
_psf dictionary and use the key,value as the arguments to
set_psf call(s).

There is still something weird going on with the warning messages
for this test. I don't understand but have got it to a point
where it works for me.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 25, 2023
The problem is that we do not include the "set_psf" call in the
ASCII serialization. Since it's not obvious what the text will be
we can't check the output serialization, but we can at least
check that the PSF has not been applied when we restore the
saved file.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Sep 25, 2023
This was a logical error in the existing code, coupled with
incomplete testing.

The fix is simple: we just need to loop through the session's
_psf dictionary and use the key,value as the arguments to
set_psf call(s).

There is still something weird going on with the warning messages
for this test. I don't understand but have got it to a point
where it works for me.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants