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

Discussing some choices made so far #7

Open
cjdoris opened this issue Feb 11, 2021 · 1 comment
Open

Discussing some choices made so far #7

cjdoris opened this issue Feb 11, 2021 · 1 comment

Comments

@cjdoris
Copy link

cjdoris commented Feb 11, 2021

Thanks for starting this package. I've been getting used to it before adding my stuff.

Here are a few things I've noticed in the implementation so far that I think need to be justified or addressed:

  1. Is NotANumber really necessary? Why not just raise an exception?
  2. I don't think we should have constructors RealInfinity(signbit::Bool) and ComplexInfinity(angle::Real): the convention in Julia Base is that for a numeric type T that T(x::Number) is the same as convert(T, x). There should be constructors private to the module called realinffromsignbit or something.
  3. Why is there checked arithmetic when this is for integers, and infinity is not an integer?
  4. Why is fld(x, Infinity()) == -1 when x<0? I am guessing that you are defining fld(x, Infinity()) as the limit as y gets big of fld(x, y), but an equally good definition is simply that it is floor(x / Infinity()) == 0. The latter definition is the one in the Julia documentation, and arguably the one people would expect.
  5. Given ComplexInfinity is not ordered, why are mod, div, fld, isless, <, min, etc. defined? They aren't defined for Complex.
@dlfivefifty
Copy link
Member

  1. Is NotANumber really necessary? Why not just raise an exception?

For consistency with Inf. Functions do not expect 0/Inf to throw an exception so if 0/∞ did it would break code.

  1. don't think we should have constructors RealInfinity(signbit::Bool)...

Agreed.

  1. Why is there checked arithmetic when this is for integers, and infinity is not an integer?

You are right, this should be for InfiniteCardinal{0} (the code was copied from InfiniteArrays.jl where Infinity <: Integer)

  1. I am guessing that you are defining fld(x, Infinity()) as the limit…

Yes. This was probably also meant for InfiniteCardinal{0} where probably the behaviour is required for some part of the array code. Once I get InfiniteArrays.jl depending on this package we can try changing things to see if it breaks it.

  1. Given ComplexInfinity is not ordered...

Note its only defined for ComplexInfinity{Bool} which we know is real, but I agree its confusing. This was copied over from OrientedInfinity{Bool} which was less controversial, but I believe predates the creation of RealInfinity, so in the past -∞ was OrientedInfinity(true). So this code can likely be deleted. It might impact ApproxFun.jl so again lets keep it for now and can try removing it once the dependencies are added.

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

No branches or pull requests

2 participants