Skip to content

MAINT make Scattering2D compatible with TF>=2.5#883

Merged
MuawizChaudhary merged 3 commits intokymatio:devfrom
lostanlen:fix-775-2D
Jun 18, 2022
Merged

MAINT make Scattering2D compatible with TF>=2.5#883
MuawizChaudhary merged 3 commits intokymatio:devfrom
lostanlen:fix-775-2D

Conversation

@lostanlen
Copy link
Collaborator

@lostanlen lostanlen commented Jun 15, 2022

Addresses #775 for 2D

@lostanlen lostanlen changed the title [WIP] make Scattering2D compatible with TF>=2.5 [WIP] make Scattering2D compatible with TF>=2.5 Jun 15, 2022
MuawizChaudhary
MuawizChaudhary reviewed Jun 15, 2022
View reviewed changes
MuawizChaudhary
MuawizChaudhary reviewed Jun 15, 2022
View reviewed changes
Copy link
Collaborator

@MuawizChaudhary MuawizChaudhary left a comment

Choose a reason for hiding this comment

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

this should be correct.

MuawizChaudhary
MuawizChaudhary reviewed Jun 15, 2022
View reviewed changes
@lostanlen
Copy link
Collaborator Author

lostanlen commented Jun 15, 2022

thanks for your help, @MuawizChaudhary . bugfixed

MuawizChaudhary
MuawizChaudhary approved these changes Jun 15, 2022
View reviewed changes
Copy link
Collaborator

@MuawizChaudhary MuawizChaudhary left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll wait for the tests to finish runnin.

@janden
Copy link
Collaborator

janden commented Jun 15, 2022

Can we add a test that fails without this fix (i.e., calls model.summary() or something)? Would be good to check that we've actually fixed it…

@lostanlen
Copy link
Collaborator Author

lostanlen commented Jun 15, 2022

Can we add a test that fails without this fix (i.e., calls model.summary() or something)? Would be good to check that we've actually fixed it…

@MuawizChaudhary can you help? looks like you were testing this on a Colab ...

@MuawizChaudhary
Copy link
Collaborator

MuawizChaudhary commented Jun 15, 2022

I'll try to get to it tonight.

@lostanlen
Copy link
Collaborator Author

lostanlen commented Jun 15, 2022

what's happening with Jenkins?

@lostanlen lostanlen added the 2D Issue with 2D scattering code label Jun 15, 2022
MuawizChaudhary added a commit to MuawizChaudhary/kymatio that referenced this pull request Jun 16, 2022
@MuawizChaudhary
TST adding model.summary() to keras 2d tests. Fails without rebasing to
ffa8b43 
kymatio#883
@MuawizChaudhary MuawizChaudhary mentioned this pull request Jun 16, 2022
Merged
@MuawizChaudhary
Copy link
Collaborator

MuawizChaudhary commented Jun 16, 2022

@janden I created PR #888 resolving the lack of tests. They fail prior to the merges.

@MuawizChaudhary
Copy link
Collaborator

MuawizChaudhary commented Jun 16, 2022 •
edited
Loading

This PR goes a small way of resolving issue #817 , as seen in this notebook, the keras example fails, but here on @lostanlen's fix-775-2D branch it works!

The tests in #888 also ensure correctness.

Lets merge soon.

@MuawizChaudhary MuawizChaudhary changed the title [WIP] make Scattering2D compatible with TF>=2.5 MAINT make Scattering2D compatible with TF>=2.5 Jun 16, 2022
janden
janden approved these changes Jun 16, 2022
View reviewed changes
Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

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

So what about 1D?

@MuawizChaudhary
Copy link
Collaborator

MuawizChaudhary commented Jun 16, 2022

@janden I merged 1d in #882

@janden
Copy link
Collaborator

janden commented Jun 17, 2022

@janden I merged 1d in #882

Got it.

@MuawizChaudhary MuawizChaudhary merged commit 3b5ed23 into kymatio:dev Jun 18, 2022
changhongw pushed a commit that referenced this pull request Jun 20, 2022
@MuawizChaudhary @lostanlen
TST add summary call to keras 1d and 2d tests. (#888)
d761ec5 
* TST adding model.summary() to keras 1d tests. Fails without rebasing to dev.

* TST adding model.summary() to keras 2d tests. Fails without rebasing to #883

* STY add space

* STY indentation

* TST now assert

* TST now assert

* fix Keras tests

Co-authored-by: Vincent Lostanlen <vincent.lostanlen@ls2n.fr>
@janden janden mentioned this pull request Jun 23, 2022
Merged
@lostanlen lostanlen deleted the fix-775-2D branch June 25, 2022 16:22
eickenberg pushed a commit that referenced this pull request Jul 5, 2022
@lostanlen @eickenberg
MAINT make Scattering2D compatible with TF>=2.5 (#883)
762c377 
* make Scattering2D compatible with TF>=2.5

* update 2d filter_bank loops

* bugfix
eickenberg pushed a commit that referenced this pull request Jul 5, 2022
@MuawizChaudhary @lostanlen @eickenberg
TST add summary call to keras 1d and 2d tests. (#888)
b41d213 
* TST adding model.summary() to keras 1d tests. Fails without rebasing to dev.

* TST adding model.summary() to keras 2d tests. Fails without rebasing to #883

* STY add space

* STY indentation

* TST now assert

* TST now assert

* fix Keras tests

Co-authored-by: Vincent Lostanlen <vincent.lostanlen@ls2n.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@janden janden janden approved these changes

@MuawizChaudhary MuawizChaudhary MuawizChaudhary approved these changes

Assignees

No one assigned

Labels

2D Issue with 2D scattering code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lostanlen @janden @MuawizChaudhary