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.

Sign up for GitHub

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

Jump to bottom

More indices generalizations #17816

Merged
merged 4 commits into from
Aug 5, 2016
Merged

More indices generalizations #17816

merged 4 commits into from
Aug 5, 2016

Conversation

timholy
Copy link
Member

@timholy timholy commented Aug 4, 2016

More complete coverage of iterators/generators/collect, coverage of transpose and ctranspose, @test_approx_eq, findn, and findnz.

@timholy timholy mentioned this pull request Aug 4, 2016
Closed
@kshyatt kshyatt added arrays [a, r, r, a, y, s] testsystem The unit testing framework and Test stdlib labels Aug 4, 2016
@timholy timholy merged commit f9fb4da into master Aug 5, 2016
@timholy timholy deleted the teh/indices_iters branch August 5, 2016 16:40
tkelman
tkelman reviewed Aug 5, 2016
View reviewed changes
base/abstractarray.jl

Represents the array `y` as an array having the same indices type as `x`.
"""
of_indices(x, y) = similar(dims->y, oftype(indices(x), indices(y)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're only calling this in 2 places, I worry that people might start calling it and hoping it doesn't change or go away

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's basically a test to see if this is something generally useful. Since it's not exported, isn't that reasonable?

I could delete the docstring if you think it would be better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting docs seems like always a bad choice unless they are factually wrong. It is not in the manual so should be fine imo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs is better than no docs, though I guess being documented in the manual has been our rough guideline for when a non-exported API is designed to be called that way

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's way too early to put this in the manual---for now this is purely an internal method, and its absence from the manual indicates that pretty clearly I think.

@tkelman
Copy link
Contributor

tkelman commented Aug 5, 2016

purely an internal method

My point is these have a habit of not staying that way - look at all the trouble promote_op has gotten us into.

@timholy
Copy link
Member Author

timholy commented Aug 5, 2016

Not the right comparison. promote_op was deliberately intended to be extended---the whole point was that packages would specialize it to be able to do math with unconventional numbers. (We added it only after having gone for 3 years with no way to do that and no other solution on the horizon. Now that inference has gotten better, it's awfully nice to get rid of promote_op.)

tkelman pushed a commit that referenced this pull request Aug 11, 2016
@timholy @tkelman
indices extensions for iterators & collect
730c0a3 
(cherry picked from commit a1e5daf)
ref #17816
tkelman pushed a commit that referenced this pull request Aug 11, 2016
@timholy @tkelman
Get (c)transpose for vectors working with non-1 indices
1debda4 
It was already working for matrices

(cherry picked from commit 9114ae6)
ref #17816
tkelman pushed a commit that referenced this pull request Aug 11, 2016
@timholy @tkelman
Get atsign-test_approx_eq working for non-1 indices
bee5a34 
(cherry picked from commit 06c6d59)
ref #17816
tkelman pushed a commit that referenced this pull request Aug 11, 2016
@timholy @tkelman
Generalize findn and findnz for non-1 indices
3d980ca 
(cherry picked from commit 37e9c5c)
ref #17816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
arrays [a, r, r, a, y, s] testsystem The unit testing framework and Test stdlib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@timholy @tkelman @KristofferC @kshyatt