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

Fix of bug in gelsd! for complex arrays and some more. #1427

Closed
wants to merge 1 commit into from
Closed

Fix of bug in gelsd! for complex arrays and some more. #1427

wants to merge 1 commit into from

Conversation

andreasnoack
Copy link
Member

Added some eps methods for complex floats. Fixed a bug in dgesld for complex types and added input argument for singularity value. Updated \ accordingly. Added some methods to handle right and left hand side of different type. Restricted methods in operators apply for Numbers only.

@ViralBShah
Copy link
Member

The xgelsd stuff looks fine. What is the rationale for changing the behaviour of eps() for Complex? About the changes in operators.jl, I think it is actually a good idea to restrict some of those to Number, but it would also be annoying to implement each operator for every new type. Also, I wonder if there might be unintended side-effects.

@JeffBezanson @StefanKarpinski What are your thoughts on the changes to operators.jl?

It would have been easier to merge this without the changes to operators.jl. Let's see what others have to say at this point, before reverting or creating a new pull request.

@nolta
Copy link
Member

nolta commented Oct 23, 2012

The operator stuff breaks the test suite:

     * linalg
no method .>(Array{Float64,1},Float64)

…complex types and added input argument for singularity value. Updated \ accordingly. Added some methods to handle right and left hand side of different type. Restricted methods in operators apply for Numbers only.
@andreasnoack
Copy link
Member Author

Sorry for not testing before sending the request. It passes all tests now.

The error was caused by .> not being defined for arrays after my changes. I have moved the definition .> (x,y) = y.<x next to > (x,y) = y < x which I think makes sense. If the restrictions in operators are problematic I can remove them, but I extrapolated from the discussion about \ and restricted the operators that did not make sense for matrices.

The changes to eps should not break anything. I changed abs to real for efficiency and added methods similar to those of eps(Float64) and eps(Float32).

@ViralBShah
Copy link
Member

eps(z::Complex) should be reverted to the original definition using abs. It is not clear what eps should do for complex numbers, and how meaningful that is. While we do have it, I think it should be matlab compatible, which is what the original behaviour was.

@andreasnoack
Copy link
Member Author

But I do not think the behaviour has changed. The output is the same for eps(real(x::Complex)) and eps(abs(x::Complex)). The only difference is that the former avoids two squarings and a square root.

@StefanKarpinski
Copy link
Sponsor Member

They're not the same:

julia> eps(abs(0.+1im))
2.220446049250313e-16

julia> eps(real(0.+1im))
5.0e-324

Matlab gives the former answer, not the latter.

@andreasnoack
Copy link
Member Author

I buy that argument! I'll revert it.

@nolta
Copy link
Member

nolta commented Oct 23, 2012

Defining eps(::Complex) seems like a matlab feature we can do without.

@andreasnoackjensen, would you mind splitting your patch into 3 parts?:

  1. the xgelsd stuff,
  2. a narrowly tailored fix for Error: stack overflow when solving linear system for different types #1418, which also adds randn(3,3)\complex(randn(3)) to the test suite,
  3. the rest of the operator changes.

@andreasnoack
Copy link
Member Author

Fair enough. I'll close this one and make some new ones.

@StefanKarpinski
Copy link
Sponsor Member

Yes, eps(::Complex) is a bit iffy since it's not clear what it means, whereas eps for floating-point is very well-defined except for the weird edge cases. We disagree with Matlab at realmax() – Matlab has eps(2.2250738585072014e-308) == 4.9407e-324, whereas we give Inf. I think our answer is technically more correct and makes the eps() function much, much faster to compute for all values since all you do is increment a register as an integer and then subtract as floats from the input value. The Matlab and C versions have crazy branches and other nonsense. Doesn't seem worth it.

@ViralBShah
Copy link
Member

I actually think we should not have eps() for Complex, or make it throw an DoesNotMakeSenseException. It was probably implemented in Matlab for some kind of customer convenience.

One could define eps(::Complex) to be the smallest complex number to add to the given complex number, so that the result is next closest to the input. However, you can have multiple answers here.

@StefanKarpinski
Copy link
Sponsor Member

LOL @ DoesNotMakeSenseException although it's also not a terrible idea.

@StefanKarpinski
Copy link
Sponsor Member

We could also add WTFDudeException and WoahThereNellyException.

@toivoh
Copy link
Contributor

toivoh commented Oct 27, 2012

I guess DomainError might be appropriate?

@ViralBShah
Copy link
Member

DomainError does seem more appropriate.

fredrikekre pushed a commit that referenced this pull request Dec 13, 2019
…34091)

git log --oneline 0c2dddd40e4d7492d2a7337be54c345011e5f1e1^..8e236a7f993f1e732ffd0ab5c15736b2594e4109

8e236a7 (HEAD -> master, origin/master, origin/HEAD) Merge pull request #1544 from JuliaLang/sk/telemetry
90b8482 telemetry: factor out telemetry file loading
228fb97 CI telemetry: send indicators for common CI env vars
246dbd0 Pkg protocol: basic anonymous opt-out telemetry
e66a75f Introduce special REPL syntax for shared environments (#1488)
afeb1ee Merge pull request #1538 from JuliaLang/sk/pkg-client-auth
9c357bb Pkg client auth: support connecting to authenticated Pkg servers
6dd7f34 PlatformEngines: revert API part of headers support (broken)
6825b48 Merge pull request #1539 from 00vareladavid/00/fixes
3f1cf40 it is invalid to `add` a package with no commits
0766765 test: default environment should be created when the primary depot does not exist
43f46f8 check no overwrite is occuring when resolving from a project file
37b6853 handle primary depot as relative path
53fdf24 Check for duplicate name/UUID input
8a6387c Remove redundant precompile statement
4d0901e Dont throw error when autocompleting faulty input (#1530)
d69f6d7 Refactor and test `Pkg.test` (#1427)
8ca8b6d PlatformEngines: use `tar -m` to ignore mtimes (#1537)
6797928 Make sure sandbox's temp Project.toml and Manifest.toml files are writable (#1534)
f968cc9 clarify: stacked envs only affect top-level loading (#1529)
0dfef59 PlatformEngines.download: add header support (#1531)
49ab53e Fix tree hashing with nested empty directories (#1522)
0c2dddd fix #1514: install_archive call in backwards_compatible_isolation (#1517)
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
…34091)

git log --oneline 0c2dddd40e4d7492d2a7337be54c345011e5f1e1^..8e236a7f993f1e732ffd0ab5c15736b2594e4109

8e236a7 (HEAD -> master, origin/master, origin/HEAD) Merge pull request #1544 from JuliaLang/sk/telemetry
90b8482 telemetry: factor out telemetry file loading
228fb97 CI telemetry: send indicators for common CI env vars
246dbd0 Pkg protocol: basic anonymous opt-out telemetry
e66a75f Introduce special REPL syntax for shared environments (#1488)
afeb1ee Merge pull request #1538 from JuliaLang/sk/pkg-client-auth
9c357bb Pkg client auth: support connecting to authenticated Pkg servers
6dd7f34 PlatformEngines: revert API part of headers support (broken)
6825b48 Merge pull request #1539 from 00vareladavid/00/fixes
3f1cf40 it is invalid to `add` a package with no commits
0766765 test: default environment should be created when the primary depot does not exist
43f46f8 check no overwrite is occuring when resolving from a project file
37b6853 handle primary depot as relative path
53fdf24 Check for duplicate name/UUID input
8a6387c Remove redundant precompile statement
4d0901e Dont throw error when autocompleting faulty input (#1530)
d69f6d7 Refactor and test `Pkg.test` (#1427)
8ca8b6d PlatformEngines: use `tar -m` to ignore mtimes (#1537)
6797928 Make sure sandbox's temp Project.toml and Manifest.toml files are writable (#1534)
f968cc9 clarify: stacked envs only affect top-level loading (#1529)
0dfef59 PlatformEngines.download: add header support (#1531)
49ab53e Fix tree hashing with nested empty directories (#1522)
0c2dddd fix #1514: install_archive call in backwards_compatible_isolation (#1517)
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.

None yet

5 participants