Skip to content

FIX examples/plot_filters.py#815

Merged
lostanlen merged 6 commits intokymatio:devfrom
danedane-haider:plot_filters
Jun 20, 2022
Merged

FIX examples/plot_filters.py#815
lostanlen merged 6 commits intokymatio:devfrom
danedane-haider:plot_filters

Conversation

@danedane-haider
Copy link
Contributor

  • Changed "Plot filters" to "Plot Frequency response of filters"
  • Added Plot for time-domain representation

@cyrusasfa
Copy link
Collaborator

looks good to me. good additions. I will test that it works.

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.

Thanks for putting this together! The main thing here is to maintain the original style of the script. Unfortunately, the latest PR (#673) destroyed the script, so you may want to look at the commit just before that PR was merged (31630ca). The goal is to have your changes be as small as possible with respect to that version.

@MuawizChaudhary
Copy link
Collaborator

@danedane-haider Do you have the bandwidth to make the necessary changes?

Edits as required:
- original formatting
- line width reduction
- psi <- psi_i
@danedane-haider
Copy link
Contributor Author

danedane-haider commented Jun 14, 2022 via email

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.

@danedane-haider One final round of edits (from my side at least) and I'm good with merging. Great job!

Copy link
Collaborator

@lostanlen lostanlen left a comment

Choose a reason for hiding this comment

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

need to rebase WRT #854 and #882

@lostanlen
Copy link
Collaborator

I have rebased and updated the comments. @danedane-haider thanks again

@codecov-commenter
Copy link

codecov-commenter commented Jun 19, 2022

Codecov Report

Merging #815 (0747e16) into dev (e89ec79) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev     #815   +/-   ##
=======================================
  Coverage   87.94%   87.94%           
=======================================
  Files          64       64           
  Lines        2257     2257           
=======================================
  Hits         1985     1985           
  Misses        272      272           
Flag Coverage Δ
jenkins_main 87.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e89ec79...0747e16. Read the comment docs.

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.

Looks good. Thanks again!

@lostanlen lostanlen changed the title Plot filters1D FIX examples/plot_filters.py Jun 20, 2022
@lostanlen
Copy link
Collaborator

@MuawizChaudhary once you approve this, we can merge

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.

@lostanlen lostanlen merged commit 8c22fe4 into kymatio:dev Jun 20, 2022
eickenberg pushed a commit that referenced this pull request Jul 5, 2022
by Daniel Haider and Vincent Lostanlen
approved by Muawiz Chaudhary and Joakim Andén

* Update plot_filters.py

* Update plot_filters.py

* Update plot_filters.py

Edits as required:
- original formatting
- line width reduction
- psi <- psi_i

* Update plot_filters.py

* update examples/1d/plot_filters

in light of #854 and #882
enable TeX rendering

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants