Skip to content

Conversation

Tokazama
Copy link
Member

@Tokazama Tokazama commented Jun 28, 2022

Why do this and what are the objectives

The primary focus here is to replace the to_parent_dims and from_parent_dims methods which were only really being used for SubArray, Adjoint, Transpose, and PermutedDimsArray. The point of those methods was originally to avoid repeatedly doing verbose extraction of the permutations for those last three arrays and to consolidate the number of generated functions for mapping across the indices of SubArray. However, there are some problems with SubArray that require more than simple dimension to dimension mapping.

The main goal here is to have something that actually produces an accurate dimension mapping for SubArray. A sort of secondary goal is to reduce many of the generated functions we've needed to support SubArray. Inference is working on local tests (Julia v1.6, v1.7, v1.8,) but seems to be failing CI on v1.6 for some reason. I guess I'll have to do some trial and error there through multiple PRs.

This is not a proposal for a formal interface at this time.
In fact, I'm just removing the formal dimension mapping that to_parent_dims and from_parent_dims composed, because it just wasn't working.
I've tried to include a lengthy explanation of how the internals work in the code comments so everyone knows what's going on as we move forward.

Points of Review

@chriselrod, it appears this is still safe for the LV stuff (which was my main concern moving forward with a release). I would think it's possible to replace the generated function for dense_dims that SubArray requires but I haven't had time to figure out what's going on with that code there yet. I welcome any ideas you have to simplify that but I probably won't have time to figure it out and replace it with something that isn't generated and is performant.
Other than that, I've checked that the methods I'm removing aren't used in any currently used version of ArrayInterface in downstream packaging.

@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #327 (93dde6c) into master (1c6c725) will decrease coverage by 0.94%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
- Coverage   91.10%   90.16%   -0.95%     
==========================================
  Files           9        9              
  Lines        1462     1342     -120     
==========================================
- Hits         1332     1210     -122     
- Misses        130      132       +2     
Impacted Files Coverage Δ
src/ArrayInterface.jl 86.06% <ø> (-0.12%) ⬇️
src/size.jl 91.89% <85.71%> (+1.37%) ⬆️
src/indexing.jl 84.37% <87.50%> (-3.58%) ⬇️
src/dimensions.jl 93.57% <90.16%> (-2.34%) ⬇️
src/stridelayout.jl 89.60% <90.36%> (-0.94%) ⬇️
src/axes.jl 92.53% <100.00%> (-0.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c6c725...93dde6c. Read the comment docs.

@Tokazama Tokazama marked this pull request as ready for review June 28, 2022 19:01
@Tokazama Tokazama requested a review from chriselrod June 28, 2022 20:58
Copy link
Collaborator

@chriselrod chriselrod left a comment

Choose a reason for hiding this comment

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

I don't think removing @generated is a high priority/it can be done in a later PR.

@Tokazama Tokazama merged commit 702e092 into master Jun 30, 2022
@Tokazama Tokazama deleted the new-dimension-mapping branch June 30, 2022 06:06
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.

2 participants