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 \ bug introduced in PR#15354 by removing parent of sub. #16473

Merged
merged 1 commit into from
May 21, 2016

Conversation

pkofod
Copy link
Contributor

@pkofod pkofod commented May 20, 2016

As @timholy pointed out in #15354 I, quite embarrassingly, introduced a bug in A\v for A being a Factorization, and v being a SubArray with complex elements, by inappropriately using parent. I have proposed a one-line fix, which does not at all use SubArrays in a good way, but it will give the correct result. I guess this is related to the whole discussion on inefficient fallback methods. I am happy to take advice and criticism if there's a better way.

edit: didn't run all tests locally, so didn't catch the problems visible at CIs. I'll try to fix it.
edit^2: fixed by defining it in the most general ("Factorization","SubArray") case.

@tkelman
Copy link
Contributor

tkelman commented May 20, 2016

The bug only applies for the complex methods immediately above, right? And isn't the bug in those issues still there? What array types other than SubArray have parent differ from the array itself? It seems like the proper fix would be removing the usage of parent, but what would that break that you needed it for in #15354?

@pkofod
Copy link
Contributor Author

pkofod commented May 20, 2016

Ah, sorry. I obviously need to remove parent again. It went something like this.

  1. The two methods didn't work for (Factorization, SubArray) (but wasn't tested before), because reinterpret doesn't work for SubArrays.
  2. I added parent because I (mistakenly) thought it would fix the problem. It didn't, but there were no tests (like the one added here) to show it.
  3. I added the new method to make sure the new test works.

@vtjnash vtjnash merged commit 83269ca into JuliaLang:master May 21, 2016
@tkelman
Copy link
Contributor

tkelman commented May 21, 2016

There's probably a more efficient way to do F\copy(B) but I guess for now it's okay.

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.

3 participants