Importing Bugs #13545 - grouping twice gives an IndexError exception about sherpa

[olaurino]

@anetasie I can reproduce this. A simple workaround is to call ungroup() before grouping again.

I guess it makes sense that data should be ungrouped before it can be regrouped, it this correct?

If it is, I need more information regarding the fix: the simplest fix I can think of involves refusing to apply a new group function on a dataset that has already been grouped, and showing the user a WARNING.

An alternative involves automatically calling ungroup() if a group function is called and the dataset has already been grouped.

I can also see possible combinations of these two possibilities (e.g. refuse to group a grouped dataset, but accept a keyword argument to force the application of ungroup()).

What do you think?

[anetasie]

I don't think we should refuse to apply the new grouping. Sherpa does not have support for hierarchical grouping, so the data needs to ungroup before applying a new group. I would say that this can be done automatically by calling ungroup(). Although, I see the reason for just refusing to apply a new grouping and exit with error. I'll discuss with SDS and come back with the final answer.

[olaurino]

Sherpa does not have support for hierarchical grouping,

Are there any plans to support hierarchical grouping in the foreseeable future?

[anetasie]

We did not plan on this and I don't think this is something with the high priority for the future.

[anetasie]

Kenny provided a good example for the automatically applying ungroup before grouping.

If you're trying to "optimize" the grouping you're likely doing something like

 group_counts(10)
 plot_data()
 group_counts(15)
 plot_data()
 group_counts(12)
 plot_data()

so this would require extra ungroup with each group() and having sherpa automatically applying ungroup makes sense.

[anetasie]

Also we will follow the XSPEC convention if we ugroup() automatically.

[olaurino]

Sounds good. Thanks for the info.

[olaurino]

Trying to come up with a minimal set of steps to reproduce the issue (and thus a test case), I realized that the issue might have more to do with filtering rather than grouping, i.e.:

In [1]: from sherpa.astro.ui import *

In [2]: load_data("sherpa-test-data/sherpatest/ciao4.3/pha_intro/3c273.pi")
WARNING: [...]

In [3]: notice(0.3,2)

In [4]: group_counts(30)

In [5]: group_counts(15)
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)

results in an error. However, the following code does not:

In [1]: from sherpa.astro.ui import *

In [2]: load_data("sherpa-test-data/sherpatest/ciao4.3/pha_intro/3c273.pi")
WARNING: [...]

In [3]: group_counts(30)

In [4]: group_counts(15)

In [5]: 

The only difference is the call to notice(0.3,2)

[anetasie]

Yes, @DougBurke suggested that there are issues with filtering and grouping, since Sherpa always starts grouping from the first channel in the data regardless of the applied filter, while when the data is filtered the user expectation is that the grouping would be only applied to the data within the valid filter.
So the logic here may need to be reviewed in more details.

[olaurino]

@anetasie The fix to this issue might actually be simpler than I expected. I am going to issue a PR as soon as I am done with the PEP8 changes. However, we should check whether we have enough testing coverage for the grouping code, especially if we plan to make significant changes.

So far, the only test exercising the group_counts method is this:
https://github.com/sherpa/sherpa-test-data/blob/master/sherpatest/ciao4.3/setfullmodel/fit.py

We should make sure that is enough. If it is not, then we should add more tests. It could be a good idea to add some unit tests, as in the above thread the grouping results are only checked against the results of the fit (or they are not checked at all). So, even if the tests may cover the grouping code, it is not clear whether meaningful tests are actually executed.

In any case I created a new test as part of the patch for this bug.

In lieu of a proper PR, here is the fix:
https://github.com/sherpa/sherpa/compare/bug-%2338-grouping-twice

[DougBurke]

Just to note that this is a slightly tricky bug to tickle, since it doesn't happen if (using the example from #38 (comment) ) I don't see the error if I use ui.notice(0.5, 8) rather than ui.notice(0.3, 2).