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

ArrayInterface integration #25

Merged
merged 7 commits into from
Nov 9, 2020
Merged

ArrayInterface integration #25

merged 7 commits into from
Nov 9, 2020

Conversation

Tokazama
Copy link
Contributor

@Tokazama Tokazama commented Nov 6, 2020

I'm not sure if you want this package to depend on ArrayInterface.jl or the other way around, but this is at least a start for getting .. as part of the the new indexing pipeline in ArrayInterface.

@ChrisRackauckas
Copy link
Member

Can you up the version and test bounds to v1.5?

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 8, 2020

I assume you wanted me to also update the projects Julia dep to 1.5

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
[deps]
ArrayInterface = "4fba245c-0d91-5ea0-9b3e-6abc04ee57a9"
Copy link
Member

Choose a reason for hiding this comment

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

this isn't needed here since it's in the project

test/Project.toml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@ChrisRackauckas ChrisRackauckas merged commit 69306cf into SciML:master Nov 9, 2020
@ChrisRackauckas
Copy link
Member

Thanks!

@timholy
Copy link

timholy commented Sep 14, 2021

Any chance this could be done via a glue package? ("Oh, just gimmee that type piracy 𝅘𝅥𝅮 𝅘𝅥𝅮 𝅘𝅥𝅮...") Or maybe even flip the dependencies, and make ArrayInterface import EllipsisNotation? See JuliaMath/IntervalSets.jl#57 (comment).

@Tokazama
Copy link
Contributor Author

I'm not against having ArrayInterface.jl depend on EllipsisNotation.jl, but isn't the point of ArrayInterface.jl to have things depend on it? We already have about ~300 lines of code that are just using @require.

@timholy
Copy link

timholy commented Sep 14, 2021

Can you clarify what .. means here? I don't see it mentioned in the documentation for ArrayInterface. Is it essentially an n-splatted :?

@Tokazama
Copy link
Contributor Author

Can you clarify what .. means here? I don't see it mentioned in the documentation for ArrayInterface. Is it essentially an n-splatted :?

Yes

@timholy
Copy link

timholy commented Sep 15, 2021

Gotcha. I see that it is in the documentation for this package, sorry. This package does clearly spell out that this usage is for arrays, and so perhaps it was a mistake on IntervalSet's part to try to incorporate it. It is a bit unfortunate that the twin uses (for indexing and constructing intervals, much like Colon being for both ranges and indexing) end up resulting in such different dependency stacks. The only issue is that you often want to construct intervals and do fun array indexing within the same ecosystem, and having to qualify an operator is such a pain that it would hamper usage.

I guess one option would be to split out a package called EllipsisOperator (with ~5 lines of code 😄) which could be imported by this one; IntervalSets could import just EllipsisOperator. Thoughts?

@Tokazama
Copy link
Contributor Author

Tokazama commented Sep 16, 2021

This kind of issue is part of what has resulted in a bunch of partially functioning successors to AxisArray. Ideally we would have one interface that everyone would feel comfortable depending on, which would make dependencies in this situation acceptable. In the absence of a common interface we now have multiple distinct implementations with their own assumed interface. The EllipsisOperator solution is probably the most polite approach, allowing us to punt the issue of common interfaces and dependencies down the road a bit. However, at a certain point people are going to just need to accept that being part of a larger ecosystem involves inheriting a code base.

I have been (and will continue to) clean up code and document more of ArrayInterface. Also, there's a possibility that overtime more traits will move to base. I've tried to explain elsewhere that this means an increasing portion of the lines there are going to be low maintenance documentation, but at the end of the day people just don't like to depend on stuff.

EDIT: Apologies if this comment was more of a rant than a productive thought. My opinion is to just leave it as is because there will always be someone that wants a single line of code out of the 1,000; so don't worry about pleasing everyone. On the other hand, I'm not the package author here so I'll go along with whatever is decided without grumbling (anymore than I already have).

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