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

Integers in the type domain #55571

Closed
wants to merge 64 commits into from
Closed

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Aug 23, 2024

Introduce TypeDomainInteger, a subtype of Integer in the type domain. That is, each value is of singleton type.

These type domain integers are then applied to many places in Base (yet other applications are also possible).

To be clear, this should go into Base, as opposed to a user package, because:

  • the invalidations when defining a new Integer subtype are horrid
  • it should be utilized from within Base, e.g., it fixes the bug below for irrationals

Fixes #37977

Updates #34003

Closes #44538

Use `Int8` instead of `Int`, according to the principle of least power.
Protect againt unnecessary promotion.
Addition is the more basic operation, avoid subtracting unnecessarily.
@nsajko nsajko added maths Mathematical functions bugfix This change fixes an existing bug feature Indicates new feature / enhancement requests labels Aug 23, 2024
@nsajko
Copy link
Contributor Author

nsajko commented Aug 23, 2024

More tests would probably be good. Is there a way to get code coverage for the test suite, to help me with seeing which tests are missing?

@adienes
Copy link
Contributor

adienes commented Aug 23, 2024

the original motivation for static ints as I understood it was mostly stuff like ranges and shapes; you seem to have used them here almost exclusively to reimplement arithmetic and mathematical functions? Do you have any benchmarks that show this is actually useful?

  • why does this implementation improve on StaticInt from Static.jl
  • why should this implementation be in Base instead of Static.jl

also it seems a bit ambitious, to put it mildly, to label this as a bugfix.

@nsajko
Copy link
Contributor Author

nsajko commented Aug 23, 2024

the original motivation for static ints as I understood it was mostly stuff like ranges and shapes

Yeah, I do say that "yet other applications are also possible" above.

Do you have any benchmarks that show this is actually useful?

This isn't tagged with the "performance" label. The motivation is numeric/symbolic accuracy, e.g. the linked issue for the defining identity for one for irrationals.

why does this implementation improve on StaticInt from Static.jl

That package is much less type-safe. The types defined there wrap values of a specific type, but the validity is not checked by the type system. In contrast, here the integers are constructed from first principles in a bijective manner. That is, each number has only a single instance, and invalid instances aren't possible.

Another benefit of my design is that certain subsets of the integers are expressible in the type system, e.g., negative integers, nonnegative integers, positive integers, integers greater than one, etc.

why should this implementation be in Base

See the current description above.

@nsajko
Copy link
Contributor Author

nsajko commented Aug 23, 2024

also it seems a bit ambitious, to put it mildly, to label this as a bugfix.

Not sure what you mean? Have you seen the linked issue marked as fixed? Do you disagree that this PR fixes it?

@adienes
Copy link
Contributor

adienes commented Aug 23, 2024

The motivation is numeric/symbolic accuracy, e.g. the linked issue for the defining identity for one for irrationals.

yes, I saw the issue with one. but this feels like an enormous amount of code and complexity and new names in Base to fix a very mild papercut.

Another benefit of my design is that certain subsets of the integers are expressible in the type system, e.g., negative integers, nonnegative integers, positive integers, integers greater than one, etc.

This can already be done with Val. I don't see the existence of TypeDomainInteger as a benefit in and of itself --- the benefit would be performance improvements, numerical accuracy improvements, cleaner APIs, etc. and I don't see proof of any of that yet besides, like we have both acknowledged, a minor issue with one

@nsajko
Copy link
Contributor Author

nsajko commented Aug 23, 2024

This can already be done with Val.

No. It's not possible to express "all Val{N} such that N is positive" in the type system.

@nsajko
Copy link
Contributor Author

nsajko commented Aug 23, 2024

Regarding the size of the PR, I'm aware that it's huge, but I thought it best to initially submit it as-is, as otherwise the motivation perhaps wouldn't be clear. And I'm not sure what would be best to exclude if I'm going to split this up.

BTW, the commit history is relatively tidy, so it should be possible to:

  • review commit-by-commit
  • split up the PR while leaving only some commits in

@JeffBezanson
Copy link
Member

I appreciate all your work on this, but I am really not convinced we need it. I'm not sure that "fixing" #37977 is really a priority; Julia on its own is not trying to be a computer algebra system. I understand the invalidations are bad when you put something like this in a package, but we really can't use that argument when evolving the language. Base is already way too big as it is, and everybody would put their package in Base if they could for better latency. Triage etc. can have a larger discussion on this but these are my initial thoughts.

@nsajko
Copy link
Contributor Author

nsajko commented Aug 23, 2024

My view:

Why type domain integers can go into Base

  • The utility doesn't depend on a specific domain of application. It's not a domain-specific feature, just the integers.
  • The implementation is small and self-contained. The algorithms (RecursiveAlgorithms) and basic definitions (Basic and LazyMinus) are at around two hundred lines taken together (and I don't really optimize for line count while writing code). The other stuff in the PR are just applications.

Why type domain integers should go into Base

  • Having it in Base allows improving Base itself, which is not possible without piracy if the feature is in a user package
    • Many functions would naturally return a -1, 0 or 1 of singleton type, if such a type existed. When type stability allows. E.g., imag(::Real).
      • Array shapes and strides are represented as tuples of integers, where some of those integers are sometimes hardoded, or depend only on the types of the arguments of a function. Having type domain integers could presumably allow obtaining both performance and convenience here at the same time. I didn't look into these kinds of applications yet, as I thought I'd start by fixing the irrationals.
    • This isn't attempted in the PR here yet, but, as discussed previously, e.g. in the linked PR by @Tokazama, having type domain integers that subtype Integer would allow convenient range types where some parameter of the range is a constant (e.g., one of the endpoints or the length).
    • It allows fixing irrationals. A common dismissal here is that Irrational isn't worth fixing, but IMO this PR getting merged would vindicate the design of the irrationals. Together with a LazyMultiple type for representing a multiplication operation, now it'd even be possible to represent, e.g., a multiple of pi in the type system. Or the positive multiples, etc. I see @oscardssmith hints at possibilities like this in the "defining zero seriously" issue linked above.
    • Future APIs could use TypeDomainInteger in most places like where the ugly Val had been used previously.
  • A user package practically can't add subtypes to Integer, causing duplication of effort and unnecessary incompatibilities. Otherwise the package effectively breaks precompilation for all packages that depend on it, due to the invalidations.

@vchuravy
Copy link
Member

I would also encourage a package first, IIUC your design is new-ish and different from current packages that provide "Integers in the type domain". Even apart from the fact that Base is too large already, it is valuable to first proof an implementation in the wild, once code goes into Base we are stuck with it for a while.

@nsajko
Copy link
Contributor Author

nsajko commented Aug 24, 2024

I would also encourage a package first

As the commit message of the first commit here says, the core design was copied from a package of mine. That said, as discussed above and extensively discussed previously on Discourse and Github, a new subtype of Integer isn't appropriate for living outside Base. Furthermore, the vast majority of this PR are changes that would be piratical if they existed in a package. So I really don't see what you think a package could accomplish here. The point of the PR is to improve Base itself.

FTR I opened a discussion on the Discourse: https://discourse.julialang.org/t/type-domain-static-integers/118568

Additionally, I can't help but draw a comparison with the recently merged PR #54653. There we had a feature that was a new design, but the feature was, unlike here, perfectly suitable for a user package. Why the double standard?

@fingolfin
Copy link
Contributor

First off, I also appreciate that tons of effort that went into this PR.

That said, unfortunately I am not at all convinced this really is a "bug fix" other than for surface issues, i.e. the deeper underlying problem with the Irrational type remain, you just made them harder to hit.

This is also a rather deep change and I would be greatly surprised if there wasn't code broken by it. As such "bug fix" is not really the right label. It adds a major new feature.

I am in particular wary of type instability issues introduced by this. For this reason I also think some careful benchmarking would be in order -- while perhaps some cases may be faster with this PR (because the compiler can now "prove" that something is zero in a few more cases and optimize it away) I am afraid other things might get slower (because type instabilities are introduced and so the compiler can optimize code less).

The new code is also rather complex and (in my eyes) under documented, i.e. there is a lot of tricky things going on with zero comments.

Moreover, I don't find the comparison with PR #54653 very appropriate: that PR was much smaller, easier to understand, and to estimate its impact.

All in all in my eyes the potential risks of this PR far outweigh the potential gains. At least based on the very little data that was presented here so far. I'd be happy to revise my evaluation if more information becomes available regarding the cost/benefit tradeoff.

@KristofferC KristofferC removed the bugfix This change fixes an existing bug label Aug 26, 2024
@nsajko nsajko marked this pull request as draft August 26, 2024 14:16
@nsajko
Copy link
Contributor Author

nsajko commented Sep 8, 2024

A few notes before closing:

  • The fix for one for irrationals was just an example application within Base. IMO the necessity of a "type domain integer" type within base is made clear by:
  • A further argument for including type domain numbers types into Base is that Base methods like imag(::Real) or sin(::Irrational{:π}) would naturally return numbers which are instances of a singleton type if such a number type existed within Base.
  • This should live in a package for some time before it could make sense to bring a PR again. Some reasons:
  • I guess I'll try making the next PR more minimal, in the sense that only methods that are necessary for a specific improvement somewhere in Base will be included, then if that PR gets merged I'll try following up.

@nsajko nsajko closed this Sep 8, 2024
@nsajko nsajko deleted the TypeDomainIntegers branch September 16, 2024 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix pi == one(pi)*pi or update docs for one()
6 participants