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

Compact show for Frequencies #55

Merged
merged 3 commits into from
Feb 5, 2024
Merged

Compact show for Frequencies #55

merged 3 commits into from
Feb 5, 2024

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Jul 26, 2021

This PR specializes show for Frequencies. It's a workaround for JuliaLang/julia#39963, and in general this makes the output more readable while displaying frequencies obtained from fftfreq and rfftfreq.

On master

julia> f = fftfreq(5, 0.3)
5-element Frequencies{Float64}:
  0.0
  0.06
  0.12
 -0.12
 -0.06

julia> show(f)
[0.0, 0.06, 0.12, -0.12, -0.06]

julia> f = rfftfreq(5, 0.3)
3-element Frequencies{Float64}:
 0.0
 0.06
 0.12

julia> show(f)
[0.0, 0.06, 0.12]

After this PR:

julia> f = fftfreq(5, 0.3)
5-element Frequencies{Float64}:
  0.0
  0.06
  0.12
 -0.12
 -0.06

julia> show(f)
[0:2; -2:-1]*0.06

julia> f = rfftfreq(5, 0.3)
3-element Frequencies{Float64}:
 0.0
 0.06
 0.12

julia> show(f)
[0:2;]*0.06

As a consequence:

on master

julia> g() = rfftfreq(100, 1)
g (generic function with 1 method)

julia> @code_warntype g()
Variables
  #self#::Core.Const(g)

Body::Frequencies{Float64}
1%1 = Main.rfftfreq(100, 1)::Core.Const([0.0, 0.01, 0.02, 0.03, 0.04, 0.05, 0.06, 0.07, 0.08, 0.09, 0.1, 0.11, 0.12, 0.13, 0.14, 0.15, 0.16, 0.17, 0.18, 0.19, 0.2, 0.21, 0.22, 0.23, 0.24, 0.25, 0.26, 0.27, 0.28, 0.29, 0.3, 0.31, 0.32, 0.33, 0.34, 0.35000000000000003, 0.36, 0.37, 0.38, 0.39, 0.4, 0.41000000000000003, 0.42, 0.43, 0.44, 0.45, 0.46, 0.47000000000000003, 0.48, 0.49, 0.5])
└──      return %1

After this PR:

julia> @code_warntype g()
Variables
  #self#::Core.Const(g)

Body::Frequencies{Float64}
1%1 = Main.rfftfreq(100, 1)::Core.Const([0:50;]*0.01)
└──      return %1

@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (111eda5) 94.80% compared to head (41f58b5) 94.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   94.80%   94.83%   +0.02%     
==========================================
  Files           5        5              
  Lines         443      445       +2     
==========================================
+ Hits          420      422       +2     
  Misses         23       23              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jishnub
Copy link
Contributor Author

jishnub commented Aug 18, 2021

@stevengj @ararslan Could you have a look at this?

@stevengj
Copy link
Member

Looks fine to me.

@ararslan
Copy link
Member

I personally don't find it to be a significant improvement in readability but I also don't care much.

@jishnub
Copy link
Contributor Author

jishnub commented Aug 20, 2021

To me this seems to be a quality-of-life improvement, as it makes the output of @code_warntype significantly more readable on terminals with word wrap. Without this, the constant may span several pages of terminal output making it pretty hard to scroll through and find useful information.

Even otherwise, it's easier to get the number of frequency bins from this display format.

On master:

julia> f = fftfreq(10, 3);

julia> show(f)
[0.0, 0.3, 0.6, 0.8999999999999999, 1.2, -1.5, -1.2, -0.8999999999999999, -0.6, -0.3]

This PR:

julia> show(f)
[0:4; -5:-1]*0.3

One may obtain, for example, the index of the Nyquist frequency without the need to count.

@mcabbott
Copy link

mcabbott commented Sep 3, 2021

An alternative would be to print fftfreq(5, 0.3), the constructor. That's like what zip does.

@jishnub
Copy link
Contributor Author

jishnub commented Feb 3, 2024

Gentle bump. I'm not sure about using functions to represent the types, since this can't be uniquely determined for rfftfreq.

julia> rfftfreq(4,4) == rfftfreq(5,5)
true

Perhaps it's better to represent the exact form of the constructor, although that might be a bit cryptic if one doesn't know what the fields are. E.g.:

julia> f = rfftfreq(4, 4); show(f)
Frequencies(3, 3, 1.0)

The original suggestion in the PR is more readable, but the constructor will firstly communicate the type, and secondly, it will always create the correct object (which is what the docstring of show recommends). In either case, we probably do want to go ahead with this PR.

@jishnub jishnub merged commit d743eb6 into JuliaMath:master Feb 5, 2024
15 checks passed
@jishnub jishnub deleted the showFreq branch February 5, 2024 12:26
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.

4 participants