MAINT deprecate `average` in `Scattering1D` by cyrusasfa · Pull Request #897 · kymatio/kymatio

cyrusasfa · 2022-06-21T12:35:47Z

Addressing #885. Deprecation of Scattering1D average for v0.3, in preparation for removal in v0.4

Alters the semantics of T:

T=None performs default mode averaging, i.e. T=2**J
T=0 is no averaging
Any other integer T will stride over the specified support

I have made an average property and renamed the attribute to self._average

UPDATE: 25/06/22

reinstated the average kwarg and attribute.
allows T=0 iff average=False, else throws exception

changhongw · 2022-06-22T15:00:21Z

T=None performs default mode averaging, i.e. T=2**J

T=0 is no averaging

Any other integer T will stride over the specified support

Why not using T=2**J as the default rather than T=None (as following)? Having both T=None and T=0 may cause confusion for the case of no averaging?

T=0: no averaging
T=2**J: default averaging
T=other positive integer: other averaging

cyrusasfa · 2022-06-22T15:47:05Z

T=None performs default mode averaging, i.e. T=2**J

T=0 is no averaging

Any other integer T will stride over the specified support

Why not using T=2**J as the default rather than T=None (as following)? Having both T=None and T=0 may cause confusion for the case of no averaging?
* `T=0`: no averaging

* `T=2**J`: default averaging

* `T=other positive integer`: other averaging

good idea but you cannot write def __init__(J, shape, T=2**J ... ). that would be referencing a variableJ that is undefined.

changhongw · 2022-06-22T19:57:32Z

T=None performs default mode averaging, i.e. T=2**J

T=0 is no averaging

Any other integer T will stride over the specified support

Why not using T=2**J as the default rather than T=None (as following)? Having both T=None and T=0 may cause confusion for the case of no averaging?
* `T=0`: no averaging

* `T=2**J`: default averaging

* `T=other positive integer`: other averaging
good idea but you cannot write def __init__(J, shape, T=2**J ... ). that would be referencing a variableJ that is undefined.

Indeed.

cyrusasfa · 2022-06-25T20:43:33Z

OK the average kwarg is back and is an attribute. T=0 is only accepted if average=False

lostanlen · 2022-06-25T20:53:06Z

@cyrusvahidi can't we default average to None so that it becomes False internally if and only if T=0?
With your latest change, the setting T=0 has no justification because it must be paired with passing average=False

the issue raised by @janden was different:

if user passes average=False and leaves T=None, we raise a warning and perform averaging. Instead, we should not average

or maybe i'm misunderstanding something?

MuawizChaudhary

just one set of changes

cyrusasfa · 2022-06-25T21:00:29Z

this seems safe to me and also a better way to send the deprecate message. as long as we check not average and T == 0. do you agree?

lostanlen · 2022-06-25T21:02:31Z

this seems safe to me and also a better way to send the deprecate message. as long as we check not average and T == 0. do you agree?

it's certainly safe now but i'd argue it's too safe in the sense that (if i understand well " T=0 is only accepted if average=False") the average argument will remain compulsory (=False) if one wants to disable averaging. Just passing T=0 is no longer enough.

cyrusasfa · 2022-06-25T21:03:54Z

this seems safe to me and also a better way to send the deprecate message. as long as we check not average and T == 0. do you agree?

it's certainly safe now but i'd argue it's too safe in the sense that (if i understand well " T=0 is only accepted if average=False") the average argument will remain compulsory (=False) if one wants to disable averaging.

yes, too safe. So only error if T=0 and average=True. average=False will override any T.

MuawizChaudhary

@lostanlen what do ya think?

lostanlen · 2022-06-25T21:44:25Z

"average=False will override any T" is still not right. Here's what i have in mind, presented as a truth table

⬇️ `average` / `T` ➡️	`None`	`0` (int)	`>=1` (int or float)
`None`	avg. with `T=2**J`	no avg.	avg. with user-set `T`
`True`	avg. with `T=2**J` (`DeprecationWarning`)	`ValueError`	avg. with user-set `T` (`DeprecationWarning`)
`False`	no avg. (`DeprecationWarning`)	`ValueError`	`ValueError`

The new default will be average=None, T=None, and will be backwards compatible with the old default, without warning nor error.
Passing T=0 and letting average=None will be the new way to specify average=False.
Already-in-place settings average=False and T>=1 will keep working as intended. The former will raise a DeprecationWarning, instructing the user to replace average=False by T=0 before v0.4.
Passing average=True together with T>=1 or T=None will raise a DeprecationWarning, instructing the user to remove the average argument befor v0.4.
Passing T=0 together with setting average=False will raise a ValueError, because this is redundant. The ValueError will instruct the user to remove the average argument immediately.
If both parameters are set (to non-None values), neither of the two parameters will override the other. T=0, average=True will error, and same for T>=1, average=False.

We have four possible combinations as of v0.2. With this PR, we will have nine, of which three are errors and three are deprecated.
In the future (v0.4), we will remove average and only have (9-3-3) = 3 branches: T=None, T=0, and T>=1.

I haven't included this in the truth table, but cases T<0 and 0<T<1 should also lead to ValueError's.

This is a good first step towards supporting T="global" in v0.4

lostanlen · 2022-06-27T12:10:16Z

        else:
            self.average = True if self.average is None else self.average 
+            if not self.average: 
+                raise ValueError("T must be None or 0 when average=False"


i'm not sure what we're doing here. we're in a T>=1 branch, so the logic i'm hoping to see is:
if average==None --> no lizard
if average==True --> DeprecationWarning. Tell the user to remove average kwarg, and that averaging will be applied.
if average==False --> ValueError. Tell the user to remove average kwarg to apply averaging at scale T. Tell the user to set T=0 to disable averaging.

This ValueError seems to suggest that T should be modified and average should be kept. That code will break in v0.4 since we'll remove the average kwarg. So right now, we want to discourage the use of average in all situations.

in this case if average is None then indeed no lizard. if False then there's a value error. I updated the message too.

lostanlen

looks perfect now !!

author: Cyrus Vahidi reviewers : Muawiz Chaudhary, Vincent Lostanlen * scat1d frontend: add deprecated average property * deprecate average documentation * new semantics for T and remove average kwargs * average default None kwargs * fix test Ux * fix scattering1d Ux tests * reinstate 1d average attribute * check T is integer * update deprecation warning msg * default average to None * deprecation message: * remove empty line * raise value error if average is False and T is not None or 0 * change valueerror message when average=False and T>=1 * update deprecation warning message * update deprecation warning message * update deprecation warning message * update value error message

cyrusasfa added 6 commits June 20, 2022 22:40

scat1d frontend: add deprecated average property

169fc48

deprecate average documentation

0a4660e

new semantics for T and remove average kwargs

3d80461

average default None kwargs

e94f96f

fix test Ux

040fe31

fix scattering1d Ux tests

2c9b8e7

cyrusasfa added the 1D Issue with 1D scattering code label Jun 21, 2022

cyrusasfa added this to the 0.3.alpha milestone Jun 21, 2022

cyrusasfa requested review from MuawizChaudhary, changhongw and lostanlen June 21, 2022 12:35

changhongw reviewed Jun 22, 2022

View reviewed changes

Comment thread kymatio/scattering1d/frontend/base_frontend.py Outdated

Comment thread kymatio/scattering1d/frontend/base_frontend.py Outdated

reinstate 1d average attribute

d2c634a

cyrusasfa marked this pull request as draft June 25, 2022 20:41

cyrusasfa requested a review from janden June 25, 2022 20:41

cyrusasfa added 2 commits June 25, 2022 21:48

check T is integer

68471dc

update deprecation warning msg

ced16d7

MuawizChaudhary requested changes Jun 25, 2022

View reviewed changes

Comment thread kymatio/scattering1d/frontend/base_frontend.py Outdated

Comment thread tests/scattering1d/test_torch_scattering1d.py

default average to None

9cf4a22

cyrusasfa marked this pull request as ready for review June 25, 2022 21:15

MuawizChaudhary approved these changes Jun 25, 2022

View reviewed changes

lostanlen requested changes Jun 25, 2022

View reviewed changes

cyrusasfa added 3 commits June 27, 2022 10:13

deprecation message:

48f71ca

remove empty line

d69a396

raise value error if average is False and T is not None or 0

782813e

lostanlen requested changes Jun 27, 2022

View reviewed changes

cyrusasfa added 5 commits June 27, 2022 16:31

change valueerror message when average=False and T>=1

37b3bc5

update deprecation warning message

8482ecf

update deprecation warning message

553f518

update deprecation warning message

6632f63

update value error message

36dc7a9

lostanlen approved these changes Jun 27, 2022

View reviewed changes

lostanlen merged commit fe6ac19 into kymatio:dev Jun 27, 2022

This was referenced Aug 13, 2022

average should be None by default in v0.3 #917

Closed

FIX (v0.3.X backport) set average=None in Scattering1D frontends #924

Merged

Conversation

cyrusasfa commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changhongw commented Jun 22, 2022

Uh oh!

cyrusasfa commented Jun 22, 2022

Uh oh!

changhongw commented Jun 22, 2022

Uh oh!

Uh oh!

Uh oh!

cyrusasfa commented Jun 25, 2022

Uh oh!

lostanlen commented Jun 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MuawizChaudhary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cyrusasfa commented Jun 25, 2022

Uh oh!

lostanlen commented Jun 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyrusasfa commented Jun 25, 2022

Uh oh!

MuawizChaudhary left a comment

Choose a reason for hiding this comment

Uh oh!

lostanlen commented Jun 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lostanlen Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyrusasfa Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostanlen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cyrusasfa commented Jun 21, 2022 •

edited

Loading

lostanlen commented Jun 25, 2022 •

edited

Loading

lostanlen commented Jun 25, 2022 •

edited

Loading

lostanlen commented Jun 25, 2022 •

edited

Loading

lostanlen Jun 27, 2022 •

edited

Loading

cyrusasfa Jun 27, 2022 •

edited

Loading