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

Fit class: what to do about changing the stats class? #2063

Closed
DougBurke opened this issue Jun 17, 2024 · 2 comments · Fixed by #2054
Closed

Fit class: what to do about changing the stats class? #2063

DougBurke opened this issue Jun 17, 2024 · 2 comments · Fixed by #2054

Comments

@DougBurke
Copy link
Contributor

This has come out of the discussion at #2054 (comment)

It could be considered a bug, a design decision, a new enhancement, or ...

The issue is that when directly using the Fit class you can change the statistic in use, but an internally-controlled object doesn't know about this change:

>>> from sherpa.data import Data1D
>>> from sherpa.models.basic import Scale1D
>>> from sherpa.stats import Cash
>>> d = Data1D('', [1, 2, 3], [3, 7, 2])
>>> m = Scale1D()
>>> from sherpa.fit import Fit
>>> f = Fit(d, m)
>>> f.fit()
<Fit results instance>
>>> f.stat.name, f._iterfit.stat.name
('chi2gehrels', 'chi2gehrels')

You don't need to have called the fit method to show this. The _iterfit attribute is a "hidden" part of Fit.

Now, I can change the stat attribute

>>> f.stat = Cash()
>>> f.stat.name, f._iterfit.stat.name
('cash', 'chi2gehrels')

However, note that _iterfit has not been updated. I believe that it should be updated too (ideally we'd be able to source the information from the same location but the current design makes that hard, as would - I think - making _iterfit.stat a read-only attribute to make sure users can't say f.stat = CStat(); f._iterstat.stat = Chi2DataVar();).

However, I am not 100% sure about this (I note that we only have two tests which would be affected by this behaviour).

I do not believe this is a problem for the UI layer since in the UI layer when we need a Fit object we create one on the fly, via the internal _get_fit method, so we never get into this situation where the stat field can get changed.

@hamogu
Copy link
Contributor

hamogu commented Jun 17, 2024

I think it's a design problem and should be fixed, if possible with low effort.
We need find way for _iterfit to retain a reference to the "parent" Fit object so that we can just call whatever the statistic in that object is.

DougBurke added a commit to DougBurke/sherpa that referenced this issue Jun 17, 2024
The issue at hand is that if you *change* the stat method of the
Fit structure then you will get a mis-match with the actual
fitted statistic, which remains using the _iterfit.stat method
(i.e. the original statistic).

We only have two test cases that expose this difference, so take
advantage of the new "write the fit results to StingIO" capability
and show that the stat value has changed (i.e. all but the last row
use the original statistic, in this case Chi2DataVar, so the stat
value is ~ 8500, and only the last row uses the Cash or CStat
value).

This is intended as a regression test.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Jun 17, 2024
We can make sure that changing the Fit stat also changes the
associated _iterfit.stat field, which seems like it should be
the correct thing to do.

There are only two tests where this is an issue, and it's unclear
whether the changes are good, bad, or not significant, since

a) the tests appear to be regression tests
b) we don't know how the test data was created, so we don't know
   what the true values are

   Note that the parameter values do not appear to have changed
   "hugely", but I haven't looked into the results too deeply.

The tests do show the change - in that the statistic value reported
in the "fit(outfile=...)" option now remains consistent (i.e.
matches the expected value, assuming our new interpretation of
the stat field of the Fit object is correct [which it should be,
I claim]).
DougBurke added a commit to DougBurke/sherpa that referenced this issue Jun 17, 2024
We can just call the callback routne after updating the model
values, rather than manually setting the thawed pars,
calculating the stat, and writing out the values to a file.
This is ony possible now that sherpa#2063 has been addressed.
@DougBurke
Copy link
Contributor Author

I ended up with a "simple" fix - when you change the stat attribute of Fit then you also change the _iterfit.stat method. It doesn't catch all cases but is a minimal fix. Ideally we'd only create the _iterfit object when needed (ie make it throwaway) but there's design issues that make this hard and I don't really want to spend too much time here as this is nerd-sniping myself to the nth degree...

I made the changes to #2054

DougBurke added a commit to DougBurke/sherpa that referenced this issue Jun 27, 2024
The issue at hand is that if you *change* the stat method of the
Fit structure then you will get a mis-match with the actual
fitted statistic, which remains using the _iterfit.stat method
(i.e. the original statistic).

We only have two test cases that expose this difference, so take
advantage of the new "write the fit results to StingIO" capability
and show that the stat value has changed (i.e. all but the last row
use the original statistic, in this case Chi2DataVar, so the stat
value is ~ 8500, and only the last row uses the Cash or CStat
value).

This is intended as a regression test.
DougBurke added a commit to DougBurke/sherpa that referenced this issue Jun 27, 2024
We can make sure that changing the Fit stat also changes the
associated _iterfit.stat field, which seems like it should be
the correct thing to do.

There are only two tests where this is an issue, and it's unclear
whether the changes are good, bad, or not significant, since

a) the tests appear to be regression tests
b) we don't know how the test data was created, so we don't know
   what the true values are

   Note that the parameter values do not appear to have changed
   "hugely", but I haven't looked into the results too deeply.

The tests do show the change - in that the statistic value reported
in the "fit(outfile=...)" option now remains consistent (i.e.
matches the expected value, assuming our new interpretation of
the stat field of the Fit object is correct [which it should be,
I claim]).
DougBurke added a commit to DougBurke/sherpa that referenced this issue Jun 27, 2024
We can just call the callback routne after updating the model
values, rather than manually setting the thawed pars,
calculating the stat, and writing out the values to a file.
This is ony possible now that sherpa#2063 has been addressed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants