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

No error for no-ops in ungroup and friends #766

Merged
merged 3 commits into from
Jun 25, 2020
Merged

Conversation

hamogu
Copy link
Contributor

@hamogu hamogu commented Mar 25, 2020

Currently, ungroup and unsubtract throw a DataErr, if a dataset is not
grouped or not subtracted. Why is this necessary?
For purely interactive use if just one or two datasets, an Exception is one way
of providing feedback to the user and remind him "hey, you did not even group
this before, are you sure you know what you are doing?".
However, when working with more datasets, it gets annoying and it's even worse
when I have a long-running script. Why does the script fail, if all I'm doing is
to accidentally ungroup something that was ungrouped before.
Here is what I do: I play around interactively, group with or that and then I
want to ungroup or unsubtract everything. Currently, I have to go one by one,
with code like this:

for i in ui.list_data_ids():
        try:
            ui.ungroup(i)
        except sherpa.utils.err.DataErr:
            # It's already ungrouped
            pass

That's clunky, so I wonder: Why is it necessary to throw an Exception when a
dataset is already ungrouped or unsubtracted?
If I want to ungroup, why would it matter to me if data was previously grouped
or not? What matters is that it's ungrouped after.

This PR removes some of those (in my opinion) annoying exceptions.

Note that this needs a through review not just for the approach, but also for the implementation. I'm new to the Sherpa internals and the fact that no test needed to be changed to make it pass locally indicates that the exceptions are not covered by tests.
(If there is agreement to proceed with this, I'll add a test.)

Currently, `ungroup` and `unsubtract` throw a `DataErr`, if a dataset is not
grouped or not subtracted. Why is this necessary?
For purely interactive use if just one or two datasets, an Exception is one way
of providing feedback to the user and remind him "hey, you did not even group
 this before, are you sure you know what you are doing?".
However, when working with more datasets, it gets annoying and it's even worse
when I have a long-running script. Why does the script fail, if all I'm doing is
to accidentally ungroup something that was ungrouped before.
Here is what I do: I play around interactively, group with or that and then I
want to ungroup or unsubtract everything. Currently, I have to go one by one,
with code like this:
```
for i in ui.list_data_ids():
        try:
            ui.ungroup(i)
        except sherpa.utils.err.DataErr:
            # It's already ungrouped
            pass
```

That's clunky, so I wonder: Why is it necessary to throw an Exception when a
dataset is already ungrouped or unsubtracted?
If I want to ungroup, why would it matter to me if data was previously grouped
or not? What matters is that it's ungrouped after.

This PR removes some of those (in my opinion) annoying exceptions.
@hamogu
Copy link
Contributor Author

hamogu commented Mar 29, 2020

Note that the current practice is asymmetric: While ungrouping an already ungrouped spectrum raises an exception, grouping an already grouped spectrum (even when using the exact same grouping) is accepted.

@anetasie
Copy link
Contributor

anetasie commented Apr 8, 2020

@hamogu Thanks, Yes, no-ops in ungroup would be good.
btw. when ungroup operates on grouped data it will group the data to reflect the new grouping. It should operate on original ungrouped data.

@hamogu
Copy link
Contributor Author

hamogu commented Apr 16, 2020

OK, added tests.

@hamogu
Copy link
Contributor Author

hamogu commented Apr 16, 2020

A related question (beyond the scope of this PR): Why is there a grouped flag for datatsets at all? Why not just use the grouping all the time and set it to use every bin initially? That would make the code a lot less complex by removing a lot of if grouped ... else ..., I guess there is a performance penalty, but is it severe enough to justify that added code complexity?

@hamogu
Copy link
Contributor Author

hamogu commented Apr 16, 2020

I verified locally that the new added tests fail without this patch.

Copy link
Contributor

@dtnguyen2 dtnguyen2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file sherpa/astro/tests/test_astro_data.py is missing the year which the file was updated in the Copyright section.

@hamogu
Copy link
Contributor Author

hamogu commented May 20, 2020

I added the new copyright year. Technically, in 2020 that copyright is by MIT and not SAO, but I'm happy to pass it on to SAO ;-)

@hamogu
Copy link
Contributor Author

hamogu commented Jun 25, 2020

@dtnguyen2 I addressed your comments a while ago. Please have a look again when you find time, it would be nice to get some stuff merged.


# If we get here, checks showed data not grouped, so set group flag
data.group()
if not (data.grouped is True):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this is intended to catch False or None- if so, pep-8 suggests using if data.grouped is not True:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is taken from the old code (line 7265, seen in diff above if you expand it a little). I'm not aware of cases where data.grouped would be none.

@wmclaugh wmclaugh merged commit 80faf67 into sherpa:master Jun 25, 2020
@wmclaugh wmclaugh added this to the 4.12.2 milestone Oct 28, 2020
@hamogu hamogu deleted the noDataErr branch November 13, 2021 20:39
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Feb 25, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not).

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Mar 2, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not).

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Mar 3, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not).

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Mar 7, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not).

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Mar 8, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not). We have separate routines added
for bin_lo/bin_hi compared to the other fields since they
appear to have different rules (e.g. the number of elements
does not have to match the independent axis), but it's hard to
be sure because there is no meannigful documentation.

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Mar 9, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not). We have separate routines added
for bin_lo/bin_hi compared to the other fields since they
appear to have different rules (e.g. the number of elements
does not have to match the independent axis), but it's hard to
be sure because there is no meannigful documentation.

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Mar 9, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not). We have separate routines added
for bin_lo/bin_hi compared to the other fields since they
appear to have different rules (e.g. the number of elements
does not have to match the independent axis), but it's hard to
be sure because there is no meannigful documentation.

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Mar 10, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not). We have separate routines added
for bin_lo/bin_hi compared to the other fields since they
appear to have different rules (e.g. the number of elements
does not have to match the independent axis), but it's hard to
be sure because there is no meannigful documentation.

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Mar 11, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not). We have separate routines added
for bin_lo/bin_hi compared to the other fields since they
appear to have different rules (e.g. the number of elements
does not have to match the independent axis), but it's hard to
be sure because there is no meannigful documentation.

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Mar 11, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not). We have separate routines added
for bin_lo/bin_hi compared to the other fields since they
appear to have different rules (e.g. the number of elements
does not have to match the independent axis), but it's hard to
be sure because there is no meannigful documentation.

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Mar 19, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not). We have separate routines added
for bin_lo/bin_hi compared to the other fields since they
appear to have different rules (e.g. the number of elements
does not have to match the independent axis), but it's hard to
be sure because there is no meannigful documentation.

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Mar 21, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not). We have separate routines added
for bin_lo/bin_hi compared to the other fields since they
appear to have different rules (e.g. the number of elements
does not have to match the independent axis), but it's hard to
be sure because there is no meannigful documentation.

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are done separately as it highly
likely they will error out differently once data validation is
added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
DougBurke added a commit to DougBurke/sherpa that referenced this pull request Apr 1, 2022
Add a couple of tests to check whether bin_lo and bin_hi validate
their lengths (they do not). We have separate routines added
for bin_lo/bin_hi compared to the other fields since they
appear to have different rules (e.g. the number of elements
does not have to match the independent axis), but it's hard to
be sure because there is no meannigful documentation.

Several tests created a DataPHA object with scalar bin_lo/hi
values (added as part of sherpa#766) which at that time did nothing
(and does nothing here), but will cause a problem once data
validation is added. So they are removed here (as the settings
are not used in the changed tests).

As part of this update the validation checks (which currently fail)
so that the handling of scalar versus sequence arguments - which
were added to track sherpa#1160 - are now done separately, even moving
to different test files, as it highly likely they will error out
differently once data validation is added.

A couple of low-level checks of the sherpa.astro.ui interface
for grouping/quality are added since the logic in these routines
should be moved to the DataPHA class as part of data validation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants