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

[1.0-dev] Store indices in CartesianRange #20974

Merged
merged 1 commit into from
Jul 10, 2017
Merged

[1.0-dev] Store indices in CartesianRange #20974

merged 1 commit into from
Jul 10, 2017

Conversation

timholy
Copy link
Member

@timholy timholy commented Mar 10, 2017

The main advantage of this change is that you no longer lose type information from CartesianRange(indices(A)). The types of the specific AbstractUnitRanges are important for similar, if you're using non-1 indices. It's also kind of nice to be able to write CartesianRange{3} rather than CartesianRange{CartesianIndex{3}}.

Because this changes the parametrization of an exported type, and also introduces a new depwarn, this is queued for after 0.6.

CC @JKrehl.

@JKrehl
Copy link
Contributor

JKrehl commented Mar 10, 2017 •
edited
Loading

Not having understood all the ends this change seeps into yet, one question:
Why are the ranges forced to use Int? There are credible use-cases for using other data types for indexing an array (sparse arrays of low occupancy, lots of small arrays).

@timholy
Copy link
Member Author

timholy commented Mar 10, 2017

Because the indices in CartesianIndex are Int. I'd rather get the conversion error in the outer loop than the inner.

@JKrehl
Copy link
Contributor

JKrehl commented Mar 10, 2017

And also changing CartesianIndex to be based on NTuple{<:Integer} is maybe to much for this PR.

@timholy timholy added this to the 1.0 milestone Mar 10, 2017
@JKrehl
Copy link
Contributor

JKrehl commented Mar 13, 2017 •
edited
Loading

@timholy
Looks good to me overall except the little utility/magic function ur_int, wouldn't it be better to use convert(AbstractUnitRange{Int}, ...) for this?

@timholy
Copy link
Member Author

timholy commented Mar 13, 2017

That's not defined currently, but heck yeah, that's a better way to "spell" that conversion. Updated.

@timholy timholy mentioned this pull request Mar 16, 2017
Merged
@tkelman
Copy link
Contributor

tkelman commented Jun 29, 2017

bump, revisit?

@timholy
Copy link
Member Author

timholy commented Jun 29, 2017

Yup, on my agenda. I'm a bit focused on finishing #22210 now, but once the dust settles there I'll rebase this.

@timholy timholy force-pushed the teh/new_cartrange branch 2 times, most recently from 140dc81 to 71f7049 Compare July 9, 2017 11:05
tkelman
tkelman reviewed Jul 9, 2017
View reviewed changes
base/deprecated.jl Outdated
@@ -1546,4 +1546,10 @@ end
# END 0.7 deprecations

# BEGIN 1.0 deprecations
function CartesianRange{N}(start::CartesianIndex{N}, stop::CartesianIndex{N})
Copy link
Contributor

Choose a reason for hiding this comment

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

0.7 section, right?

@timholy
Store indices in CartesianRange
2e0601e 
The main advantage of this change is that you no longer lose type information from `CartesianRange(indices(A))`. The types of the specific AbstractUnitRanges are important for `similar`, etc.
@timholy timholy merged commit fb666b1 into master Jul 10, 2017
@timholy timholy deleted the teh/new_cartrange branch July 10, 2017 10:03
@timholy timholy mentioned this pull request Nov 28, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.7
Development

Successfully merging this pull request may close these issues.
3 participants
@timholy @JKrehl @tkelman