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

New package: IsPositive v1.0.0 #102561

Conversation

JuliaRegistrator
Copy link
Contributor

UUID: b2621e3c-9d78-409a-a956-21e401f7880f
Repo: https://github.com/putianyi889/IsPositive.jl.git
Tree: 4cfaef64e301f64a5ea34809955d6325562a322d

Registrator tree SHA: 17aec322677d9b81cdd6b9b9236b09a3f1374c6a
Copy link
Contributor

github-actions bot commented Mar 8, 2024

Your new package pull request met all of the guidelines for auto-merging and is scheduled to be merged when the mandatory waiting period (3 days) has elapsed.

Since you are registering a new package, please make sure that you have read the package naming guidelines: https://pkgdocs.julialang.org/v1/creating-packages/#Package-naming-guidelines


If you want to prevent this pull request from being auto-merged, simply leave a comment. If you want to post a comment without blocking auto-merging, you must include the text [noblock] in your comment. You can edit blocking comments, adding [noblock] to them in order to unblock auto-merging.

@goerz
Copy link
Member

goerz commented Mar 8, 2024

[noblock] I'm not entirely sure "micropackages" like this are a good idea… but OK.

@Seelengrab
Copy link
Contributor

Isn't this just >=(0) and <(0) respectively? What benefit does having this in a package bring?

@putianyi889
Copy link

Isn't this just >=(0) and <(0) respectively? What benefit does having this in a package bring?

[noblock] Comparing against 0 requires promotion which is harder to implement than signbit. Some custom types may not support comparing against general values.

@putianyi889
Copy link

putianyi889 commented Mar 8, 2024

I'm not entirely sure "micropackages" like this are a good idea

[noblock] It does seem like an overkill, but I suppose the package name wouldn't collide with a "more normal" package and hence doesn't occupy the public resource.

@Seelengrab
Copy link
Contributor

Seelengrab commented Mar 8, 2024

[noblock] Comparing against 0 requires promotion which is harder to implement than signbit. Some custom types may not support comparing against general values.

It's just an example, you can do >=(zero(T)) and <(zero(T)) just as well to skip promotion. I don't think such personal utility functions are a good fit for General.

@putianyi889
Copy link

putianyi889 commented Mar 8, 2024

It's just an example, you can do >=(zero(T)) and <(zero(T)) just as well to skip promotion. I don't think such personal utility functions are a good fit for General.

[noblock]

julia> x=BigFloat(1)
1.0

julia> @btime !(signbit(x) || iszero(x))
  65.884 ns (0 allocations: 0 bytes)
true

julia> @btime x>=zero(BigFloat)
  97.904 ns (4 allocations: 144 bytes)
true

(explaining my second point) Some types support general comparison but there is shortcut when they encode the sign and zero information in their data structures.

On your second point, it's not a personal package. It's motivated by a request by someone else from the main Julia repository.

@Seelengrab
Copy link
Contributor

Seelengrab commented Mar 9, 2024

Your benchmarks are a good case for a PR to the main Julia repository, so that every user of BigFloat can take advantage of this when they use >= and not just those using this package. Would you like to open one?

On your second point, it's not a personal package. It's motivated by a request by someone else from the main Julia repository.

Oh, I wasn't aware - could you share a link to that conversation?

@goerz
Copy link
Member

goerz commented Mar 9, 2024

Oh, I wasn't aware - could you share a link to that conversation?

I'm assuming the JuliaLang/julia#35067 linked in the README

@goerz
Copy link
Member

goerz commented Mar 9, 2024

It does seem like an overkill, but I suppose the package name wouldn't collide with a "more normal" package and hence doesn't occupy the public resource.

“Polluting the namespace” isn’t really what concerns me about micropackages. The problem is the cost of adding a dependency to a project, see, e.g. https://lucumr.pocoo.org/2019/7/29/dependency-scaling/ and the earlier post linked therein.

In Julia, the problem is even worse because a project cannot contain multiple versions of the same dependency. Thus, the more dependencies there are, the higher the risk of non-instantiable projects due to dependency conflicts. That’s why we should avoid normalizing micropackages in General.

@putianyi889
Copy link

a PR to the main Julia repository

Practically, getting a PR to be merged to the Julia base is much harder than package registration. I had a few attempts before but they mostly don't work.

due to dependency conflicts

Are there cases when the dependabot can't handle? Micropackages wouldn't have many versions thus the version conflict is less likely. If you had a package which is basically a collection of micropackages, then the frequency of updating it is much higher and thus it's easier to have a version conflict. I think it's also why large projects tend to build on a family of small packages.

when they use >=

It's going off-topic, but the problem is that >= is just more general and thus slower. I don't know how to add a method to operate on a special value rather than a type.

@Seelengrab
Copy link
Contributor

I'm assuming the JuliaLang/julia#35067 linked in the README

Thanks, I missed that. I don't see anyone asking for this to be a package; the only mention of "General" is the phrase "in general", which is an idiom. In this case, it's referring to having constant propagation working for all types, which is not possible for all BigFloat due to its dependence on ccall.

It's going off-topic, but the problem is that >= is just more general and thus slower. I don't know how to add a method to operate on a special value rather than a type.

For Julia types that don't have to interface with an external library like GMP (as BigFloat does), >= is already just as fast:

julia> f(x) = x >= zero(typeof(x))
f (generic function with 2 methods)

julia> g(x) = !(signbit(x) || iszero(x))
g (generic function with 2 methods)

julia> @benchmark g(x) setup=(x=rand())
BenchmarkTools.Trial: 10000 samples with 1000 evaluations.
 Range (min  max):  1.889 ns  9.610 ns  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     1.910 ns             ┊ GC (median):    0.00%
 Time  (mean ± σ):   1.915 ns ± 0.144 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

            ▄        █                                       
  ▃▁▁▁▁▁▁▁▁▃█▁▁▁▁▁▁▁▃█▁▁▁▁▁▁▁▁▅▁▁▁▁▁▁▁▁▂▃▁▁▁▁▁▁▁▂▃▁▁▁▁▁▁▁▁▂ ▂
  1.89 ns        Histogram: frequency by time       1.95 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

julia> @benchmark f(x) setup=(x=rand())
BenchmarkTools.Trial: 10000 samples with 1000 evaluations.
 Range (min  max):  1.719 ns  13.040 ns  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     1.740 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   1.742 ns ±  0.173 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

                      ▆                  █                    
  ▂▄▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃▁█▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃▁█▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂▄ ▂
  1.72 ns        Histogram: frequency by time        1.75 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

So the "performance problem" seems exclusive to the GMP backed types. Considering that these types are quite often used in calculations that are vastly more complicated and take orders of magnitude longer than 30ns (or 2ns on my machine, as timed by your benchmark above), I don't think this is worth it.

@goerz
Copy link
Member

goerz commented Mar 9, 2024

I just think it would be a bad idea for any package to pull this in as a dependency. I'd definitely flag it in any code review.

You could try to get these functions into Base. If they don't get accepted, there'd probably be a good reason. In fact, I'm a little confused as to how general these definitions are. The signbit function this is based on feels a bit obscure. So if I had MyNumberType, I would have to define a method for signbit before I could use this, right?

Or if not in Base, they might have a place in some other package like Unitful. Something where the performance-benefit of these definitions is relevant, and that's already a core dependency of packages in a particular relevant domain.

But beyond that, these definitions should be copied into projects instead of loaded from a dependency. So the right approach would be a "blog post" that is easily findable for people googling ispositive or other related keywords. The "blog post" could very well be the README of an unregistered package (with "copy these functions" in the instructions), maybe with an accompanying post on Discourse, and/or linked from relevant discussions like JuliaLang/julia#35067.

@putianyi889
Copy link

putianyi889 commented Mar 9, 2024

I would have to define a method for signbit before I could use this, right?

Interestingly Julia has the fallback
https://github.com/JuliaLang/julia/blob/53048b2ab1eb8ce412ef884a2431ca57da4dc4e6/base/number.jl#L137

so if you define a comparison operator, you can also use it.

@putianyi889
Copy link

try to get these functions into Base

JuliaLang/julia#53677
hopefully it works

@goerz
Copy link
Member

goerz commented Mar 9, 2024

Closing at @putianyi889's request. The plan is to pursue JuliaLang/julia#53677

@goerz goerz closed this Mar 9, 2024
@giordano giordano deleted the registrator-ispositive-b2621e3c-v1.0.0-bb63cfd9e2 branch March 11, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants