Deprecate `gausschebyshev` and add `gausschebyshevt`, ..., `gausschebyshevw` #123

hyrodium · 2023-09-14T14:41:16Z

We currently have gausschebyshev(n::Integer, kind::Integer), but we don't need arithmetic operations for kind parameter, i.e. some operations such as gausschebyshev(4, 2+1) is not desired.

Enum or Val can be used to solve this problem, however, just splitting and renaming the function would be enough here.

julia> gausschebyshev(5,2)  # deprecated method
([-0.8660254037844387, -0.4999999999999998, 6.123233995736766e-17, 0.5000000000000001, 0.8660254037844387], [0.13089969389957468, 0.3926990816987242, 0.5235987755982988, 0.39269908169872403, 0.13089969389957468])

julia> gausschebyshev2(5)  # new function
([-0.8660254037844387, -0.4999999999999998, 6.123233995736766e-17, 0.5000000000000001, 0.8660254037844387], [0.13089969389957468, 0.3926990816987242, 0.5235987755982988, 0.39269908169872403, 0.13089969389957468])

codecov · 2023-09-14T14:56:27Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04% ⚠️

Comparison is base (a593294) 96.65% compared to head (93f03ab) 96.62%.
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
- Coverage   96.65%   96.62%   -0.04%     
==========================================
  Files           9        9              
  Lines        1227     1244      +17     
==========================================
+ Hits         1186     1202      +16     
- Misses         41       42       +1

Files Changed	Coverage Δ
src/FastGaussQuadrature.jl	`100.00% <ø> (ø)`
src/gausschebyshev.jl	`96.87% <100.00%> (-3.13%)`	⬇️

... and 2 files with indirect coverage changes

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

dlfivefifty · 2023-09-14T15:40:13Z

I think better names would be gausschebyshevt (or just gausschebyshev), gausschebyshevu, gausschebyshevv and gausschebyshevw.

Cf chebyshevutransform:

https://github.com/JuliaApproximation/FastTransforms.jl/blob/fde025f2fe3643e8673a098fed02bc0d804dc7ed/src/chebyshevtransform.jl#L495

hyrodium · 2023-09-14T16:12:37Z

I think better names would be gausschebyshevt (or just gausschebyshev), gausschebyshevu, gausschebyshevv and gausschebyshevw.

Thank you for the suggestion! This is not a strong preference, but I thought something like gausschebyshev1 has good visibility.

hyrodium · 2023-09-15T15:53:28Z

How about gausschebyshev_u etc. or gausschebyshevU?

hyrodium · 2023-09-15T15:57:27Z

I opened a poll on slack.
https://julialang.slack.com/archives/C66NPKCQZ/p1694793325955829

hyrodium · 2023-09-16T09:11:58Z

Based on the poll, I have updated the name with gausschebyshevT, gausschebyshevU, gausschebyshevV, and gausschebyshevW.

@dlfivefifty Do you agree with this naming?

dlfivefifty · 2023-09-17T14:11:50Z

Note in ClassicalOrthogonalPolynomials.jl I use chebyshevt/chebyshevu/etc. as its consistent with SpecialFunctions.jl naming scheme eg besselj or hankelh.

Is there any precedence for using capital letters in this way? If not I think we should follow SpecialFunctions.jl. So either gausschebyshevt or gauss_chebyshevt

hyrodium · 2023-09-17T14:33:50Z

Is there any precedence for using capital letters in this way?

gausschebyshevT has better visibility than gausschebyshevt, and is consistent with $T_n$.

The difference between besselj and gausschebyshevt is their length, and the latter is a little hard to read.
I'm okay with gausschebyshevt, but other people (like in slack) would prefer other naming conventions.

gauss_chebyshevt is not consistent with gausslegendre etc.

In ideal, it would be nice to have names like SpecialFunctions.besselJ, ClassicalOrthogonalPolynomials.chebyshevT, etc., I guess. 😂

hyrodium · 2023-09-17T15:05:46Z

Or, should we rename all of gausshermite etc. to gauss_hermite etc.?

MikaelSlevinsky · 2023-09-17T15:10:19Z

Is there any good reason to deprecate the 2-arg method? Bessel functions in SpecialFunctions have besselj and besselj0 for example.

hyrodium · 2023-09-17T16:00:07Z

Is there any good reason to deprecate the 2-arg method?

My first comment (#123 (comment)) is the reason. And the 2-arg method is not friendly with the static analyzer JET.jl.

Before this PR

julia> using JET, FastGaussQuadrature

julia> f(n) = gausschebyshev(n, 3)
f (generic function with 1 method)

julia> g(n) = gausschebyshev(n, 5)  # typo
g (generic function with 1 method)

julia> @report_opt f(8)
No errors detected


julia> @report_opt g(8)  # No error!
No errors detected

After this PR

julia> using JET, FastGaussQuadrature

julia> f(n) = gausschebyshevV(n)
f (generic function with 1 method)

julia> g(n) = gausschebyshevZ(n)  # typo
g (generic function with 1 method)

julia> @report_opt f(8)
No errors detected


julia> @report_opt g(8)  # Yay!
═════ 1 possible error found ═════
┌ g(n::Int64) @ Main ./REPL[3]:1
│ runtime dispatch detected: %1::Any(n::Int64)::Any
└────────────────────

The 1-arg method would be compiled lighter than 2-arg version, but this is really a slight advantage.

Bessel functions in SpecialFunctions have besselj and besselj0 for example.

Both arguments of besselj have "mathematical" meaning, but gausschebyshev does not. (1st kind etc. for gausschebyshev are just expedient indexing)

julia> besselj(3.4, 4.2)  # Any numbers are accepted (mathematically)
0.4003115265583054

julia> besselj(0, 4.2)
-0.37655705436756765

julia> besselj0(4.2)  # Some special case has its function (maybe, special optimization inside?)
-0.37655705436756765

MikaelSlevinsky · 2023-09-17T16:40:09Z

To me, that sounds like you may have a case to add definitions, but why must we throw away the 2-arg version? Kind has a mathematical meaning; it's just that it must be one of the integers 1, 2, 3, or 4, not any other real or complex number.

hyrodium · 2023-09-17T16:53:52Z

why must we throw away the 2-arg version?

For better package design:

avoid confusion
friendly for static analyzers

We can release v0.5.2 with the deprecation message, and remove them (including besselroots #124) in the next breaking release v0.6.0.

Are there any reasons to keep the 2-arg version?

Kind has a mathematical meaning; it's just that it must be one of the integers 1, 2, 3, or 4, not any other real or complex number.

That's why I quoted like "mathematical". We can call them type-A to type-D, or something like that. There is not much meaning as an integer (i.e. no addition, no order, etc.), but it's just a label.

dlfivefifty · 2023-09-17T19:22:00Z

gausschebyshevT has better visibility than gausschebyshevt, and is consistent with $T_n$.

besselJ has better visibility than besselj and is consistent with $J_n$....

I haven't seen an actual argument for the inconsistency besides you like it better. Personally, I also like it better....but as I said I went with chebyshevt before for consistency, and I don't think it's a good idea to introduce inconsistency at this point.

hyrodium · 2023-09-18T00:52:03Z

I don't think it's a good idea to introduce inconsistency at this point.

Okay, let's change the names to gausschebyshevt for now!
We can revisit this discussion when we introduce other Gaussian quadrature methods with name which is not just "gauss$(a_name_of_a_person)".

hyrodium · 2023-09-21T09:13:42Z

@dlfivefifty @MikaelSlevinsky
Do you have any additional comments on this PR? Can I merge this?

dlfivefifty · 2023-09-21T10:17:21Z

Can you bump the minor version number?

dlfivefifty · 2023-09-21T10:31:01Z

This is a breaking change it should be the minor version, not the patch version

hyrodium · 2023-09-21T10:36:49Z

This is a breaking change it should be the minor version, not the patch version

No, the 2-arg version is still alive (and is deprecated), so we should bump the patch version.
I will remove the deprecated methods (including besselroots #124) in the next breaking release.

Related comment: #123 (comment)

dlfivefifty · 2023-09-21T13:03:44Z

Deprecations are breaking changes: it forces a user to change their code

dlfivefifty · 2023-09-21T13:04:09Z

Also exporting new functions is breaking

hyrodium · 2023-09-21T13:44:08Z

Deprecations are breaking changes: it forces a user to change their code

So, do you think we need two breaking releases to remove gausschebyshev?

v0.6.0 - deprecate gausschebyshev
v0.7.0 - remove deprecated gausschebyshev

I believe it's not breaking and we need just one breaking release. This is because package users don't have to change their code, and they just receive warnings.

Many other repositories add @deprecate macros etc. without breaking releases.
See https://github.com/search?q=%40deprecate++path%3A*.jl+language%3AJulia&type=Code&ref=advsearch&l=Julia&l= for search results, and some other PR (FluxML/Flux.jl#2328)

Also exporting new functions is breaking

I also believe this is not breaking. They may cause some name conflicts but are not considered as breaking usually. For example, Julia 1.9 exports pkgversion, but Julia v1.8 does not.

dlfivefifty · 2023-09-21T14:36:30Z

Umm you just leave the deprecation in. It can be deleted whenever another breaking change comes, there's no rush to remove the code.

But a warning can turn high-performance code into unusably slow code. And it's incredibly annoying if you are a user of another package that depends on this to suddenly have to see warnings all the time

Note the rules are a bit different once something is 1.x. The convention is to deprecate at 1.(x+1) and delete at 1.(x+2). This is because its easier to pin versions at the 1.x level.

hyrodium · 2023-09-21T14:59:44Z

But a warning can turn high-performance code into unusably slow code. And it's incredibly annoying if you are a user of another package that depends on this to suddenly have to see warnings all the time

This behavior can be handled by CLI argument --depwarn=yes or --depwarn=no (default).

julia --depwarn=yes

julia> using BenchmarkTools, FastGaussQuadrature

julia> @benchmark besselroots(0.2,5)
┌ Warning: `besselroots(ν::Real, n::Integer)` is deprecated, use `approx_besselroots(ν, n)` instead.
│   caller = var"##core#293"() at execution.jl:489
└ @ Main ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:489
BenchmarkTools.Trial: 579 samples with 1 evaluation.
 Range (min … max):  8.226 ms …  12.814 ms  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     8.504 ms               ┊ GC (median):    0.00%
 Time  (mean ± σ):   8.629 ms ± 448.184 μs  ┊ GC (mean ± σ):  0.00% ± 0.00%

    ▅█▅▃▁▁▁▂ ▁                                                 
  ▃███████████▆▅▄▅▇▅▅▄▄▅▄▄▄▃▃▄▃▄▃▂▃▃▃▁▂▁▃▁▂▂▂▂▃▂▂▂▂▃▁▃▁▂▁▁▁▁▂ ▃
  8.23 ms         Histogram: frequency by time        10.1 ms <

 Memory estimate: 12.83 KiB, allocs estimate: 93.

julia> @benchmark approx_besselroots(0.2,5)
BenchmarkTools.Trial: 10000 samples with 767 evaluations.
 Range (min … max):  164.785 ns …  1.729 μs  ┊ GC (min … max): 0.00% … 82.24%
 Time  (median):     170.714 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   181.138 ns ± 71.886 ns  ┊ GC (mean ± σ):  2.12% ±  4.83%

  ▁▆▆█▇▃▂▂▁ ▁              ▂▂▁             ▁                   ▁
  █████████████▇▇▇▆▆▆▆▆▅▆▆▇████▇▇▇▆▅▇▆▇█▇▇████▇▇▅▆▅▆▅▅▅▆▅▆▅▅▅▄ █
  165 ns        Histogram: log(frequency) by time       262 ns <

 Memory estimate: 208 bytes, allocs estimate: 2.

julia --depwarn=no

julia> using BenchmarkTools, FastGaussQuadrature

julia> @benchmark besselroots(0.2,5)
BenchmarkTools.Trial: 10000 samples with 685 evaluations.
 Range (min … max):  181.263 ns …  2.680 μs  ┊ GC (min … max): 0.00% … 82.70%
 Time  (median):     185.461 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   202.780 ns ± 82.863 ns  ┊ GC (mean ± σ):  2.05% ±  4.65%

  ▃██▄▃▃▁▁▁                ▁▅▅▂▁   ▁    ▁                      ▂
  ███████████▇▇▆▆▇▆▆▆▆▆▅▆▆▆███████████▇████▇▆▆▆▅▅▅▄▄▅▆▆▆▆▄▄▄▃▅ █
  181 ns        Histogram: log(frequency) by time       288 ns <

 Memory estimate: 208 bytes, allocs estimate: 2.

julia> @benchmark approx_besselroots(0.2,5)
BenchmarkTools.Trial: 10000 samples with 763 evaluations.
 Range (min … max):  164.336 ns …  1.219 μs  ┊ GC (min … max): 0.00% … 82.81%
 Time  (median):     167.722 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   180.819 ns ± 58.174 ns  ┊ GC (mean ± σ):  1.57% ±  4.64%

  ▆█▂▂▁           ▂     ▁   ▁                                  ▁
  ███████▇▆▆▆▅▆▆▆███▆▆▅▆██▇▇██▇▇▇▆▆▆▆▆▆▆▆▆▅▆▆▇▆▆▆▆▅▅▄▅▅▅▅▅▅▄▄▄ █
  164 ns        Histogram: log(frequency) by time       313 ns <

 Memory estimate: 208 bytes, allocs estimate: 2.

I understand 8.629 ms is a really bad performance, but 202.780 ns would be acceptable.

Note the rules are a bit different once something is 1.x. The convention is to deprecate at 1.(x+1) and delete at 1.(x+2). This is because its easier to pin versions at the 1.x level.

In Julia community, we treat x.y.z → x+1.y.z and 0.y.z → 0.y+1.z as breaking changes. See
BREAKING tags in https://github.com/JuliaRegistries/General/pulls.
If we need more detailed releases, I suggest releasing v1.0.0.

dlfivefifty · 2023-09-21T15:03:25Z

I'm sorry but changing the name of one of the main functions in a package is definitely breaking. I'm not going to tag this as a patch.

hyrodium · 2023-09-21T15:24:00Z

I'm sorry but changing the name of one of the main functions in a package is definitely breaking. I'm not going to tag this as a patch.

Hmm, can I ask two questions for clarity?

Should we really need to bump the minor version?
- We can still use gausschebyshev after this PR, so this PR does not break anything.
- Many other packages do not add deprecation as a breaking change.
Do we need two breaking releases to remove gausschebyshev?
- v0.6.0 - deprecate gausschebyshev and v0.7.0 - remove deprecated gausschebyshev
- I feel this is too verbose, and other packages that depend on FastGaussQuadrature.jl will get too many PRs from CompatHelper. (See Update required versions of Julia from 1.0 to 1.6 #113 (comment) for example)

dlfivefifty · 2023-09-21T15:29:57Z

Lets just not merge this then until there's another breaking change.

hyrodium · 2023-09-21T15:36:29Z

Okay...
Can I revert the version bump commits and merge this PR?

hyrodium · 2023-09-21T15:51:40Z

@ranocha Hi, we are planning to split gauschebyshev function into four functions such as gausschebyshevu in this PR. We can still use gauschebyshev function after this PR because it will be just deprecated in this PR.

My question is, should we make a breaking release for this deprecation?
I would like to have your opinion because you are maintaining a package that depends on FastGaussQuadrature.jl. (#113 (comment))

Thank you in advance!

ranocha · 2023-09-22T06:15:52Z

I'm not using gauschebyshev so it's fine with me. I would not consider introducing a deprecation a breaking change (in recent versions of Julia where this is handled via a command line flag) - at least based on what I've seen from other packages.

deprecate gausschebyshev and add gausschebyshev1 to gausschebyshev4

bba1710

hyrodium changed the title ~~Deprecate gausschebyshev and add gausschebyshev1 to gausschebyshev4~~ Deprecate gausschebyshev and add gausschebyshev1, ..., gausschebyshev4 Sep 14, 2023

hyrodium added 3 commits September 14, 2023 23:52

fix docs

075e68b

fix error handling for gausschebyshev

bffbd55

update docstrings

864a517

rename gausschebyshev* function

adbbf9c

hyrodium changed the title ~~Deprecate gausschebyshev and add gausschebyshev1, ..., gausschebyshev4~~ Deprecate gausschebyshev and add gausschebyshevT, ..., gausschebyshevW Sep 16, 2023

rename gausschebyshevT to gausschebyshev etc.

78f1744

hyrodium changed the title ~~Deprecate gausschebyshev and add gausschebyshevT, ..., gausschebyshevW~~ Deprecate gausschebyshev and add gausschebyshevt, ..., gausschebyshevw Sep 18, 2023

bump version to v0.5.2

93f03ab

dlfivefifty merged commit de3eb8f into JuliaApproximation:master Oct 2, 2023
11 of 12 checks passed

hyrodium mentioned this pull request Oct 10, 2023

PLEASE DO NOT COMMIT DIRECTLY ON THE master BRANCH! #128

Closed

t-bltg mentioned this pull request Jan 13, 2024

fix deprecation warnings - constraint LinearAlgebra #132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `gausschebyshev` and add `gausschebyshevt`, ..., `gausschebyshevw` #123

Deprecate `gausschebyshev` and add `gausschebyshevt`, ..., `gausschebyshevw` #123

hyrodium commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading

dlfivefifty commented Sep 14, 2023

hyrodium commented Sep 14, 2023

hyrodium commented Sep 15, 2023

hyrodium commented Sep 15, 2023

hyrodium commented Sep 16, 2023

dlfivefifty commented Sep 17, 2023

hyrodium commented Sep 17, 2023 •

edited

Loading

hyrodium commented Sep 17, 2023

MikaelSlevinsky commented Sep 17, 2023

hyrodium commented Sep 17, 2023

MikaelSlevinsky commented Sep 17, 2023

hyrodium commented Sep 17, 2023 •

edited

Loading

dlfivefifty commented Sep 17, 2023

hyrodium commented Sep 18, 2023

hyrodium commented Sep 21, 2023

dlfivefifty commented Sep 21, 2023

dlfivefifty commented Sep 21, 2023

hyrodium commented Sep 21, 2023

dlfivefifty commented Sep 21, 2023

dlfivefifty commented Sep 21, 2023

hyrodium commented Sep 21, 2023

dlfivefifty commented Sep 21, 2023

hyrodium commented Sep 21, 2023 •

edited

Loading

dlfivefifty commented Sep 21, 2023

hyrodium commented Sep 21, 2023 •

edited

Loading

dlfivefifty commented Sep 21, 2023

hyrodium commented Sep 21, 2023

hyrodium commented Sep 21, 2023 •

edited

Loading

ranocha commented Sep 22, 2023

Deprecate gausschebyshev and add gausschebyshevt, ..., gausschebyshevw #123

Deprecate gausschebyshev and add gausschebyshevt, ..., gausschebyshevw #123

Conversation

hyrodium commented Sep 14, 2023 • edited Loading

codecov bot commented Sep 14, 2023 • edited Loading

Codecov Report

dlfivefifty commented Sep 14, 2023

hyrodium commented Sep 14, 2023

hyrodium commented Sep 15, 2023

hyrodium commented Sep 15, 2023

hyrodium commented Sep 16, 2023

dlfivefifty commented Sep 17, 2023

hyrodium commented Sep 17, 2023 • edited Loading

hyrodium commented Sep 17, 2023

MikaelSlevinsky commented Sep 17, 2023

hyrodium commented Sep 17, 2023

MikaelSlevinsky commented Sep 17, 2023

hyrodium commented Sep 17, 2023 • edited Loading

dlfivefifty commented Sep 17, 2023

hyrodium commented Sep 18, 2023

hyrodium commented Sep 21, 2023

dlfivefifty commented Sep 21, 2023

dlfivefifty commented Sep 21, 2023

hyrodium commented Sep 21, 2023

dlfivefifty commented Sep 21, 2023

dlfivefifty commented Sep 21, 2023

hyrodium commented Sep 21, 2023

dlfivefifty commented Sep 21, 2023

hyrodium commented Sep 21, 2023 • edited Loading

dlfivefifty commented Sep 21, 2023

hyrodium commented Sep 21, 2023 • edited Loading

dlfivefifty commented Sep 21, 2023

hyrodium commented Sep 21, 2023

hyrodium commented Sep 21, 2023 • edited Loading

ranocha commented Sep 22, 2023

Deprecate `gausschebyshev` and add `gausschebyshevt`, ..., `gausschebyshevw` #123

Deprecate `gausschebyshev` and add `gausschebyshevt`, ..., `gausschebyshevw` #123

hyrodium commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading

hyrodium commented Sep 17, 2023 •

edited

Loading

hyrodium commented Sep 17, 2023 •

edited

Loading

hyrodium commented Sep 21, 2023 •

edited

Loading

hyrodium commented Sep 21, 2023 •

edited

Loading

hyrodium commented Sep 21, 2023 •

edited

Loading