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

add mapview() support #53

Closed
wants to merge 1 commit into from
Closed

Conversation

aplavin
Copy link
Member

@aplavin aplavin commented Jun 7, 2022

I really like this feature, and how simple it was to implement!
Transparently set values on mapped array views (from SplitApplyCombine.jl) when the mapping function is an optic.
The simplest example from tests:

X = [(a=1, b=2), (a=3, b=4)]
Y = mapview(@optic(_.b), X)
@test Y == [2, 4]
Y[2] = 100
@test X == [(a=1, b=2), (a=3, b=100)]

Haven't noticed before that Accessors.jl already uses Requires, which makes this addition possible without extra dependencies.

@@ -0,0 +1,3 @@
import .SplitApplyCombine: MappedArray

Base.setindex!(ma::MappedArray, val, ix) = parent(ma)[ix] = set(parent(ma)[ix], ma.f, val)
Copy link
Member

Choose a reason for hiding this comment

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

The feature is very neat, but this is type piracy. One possibility would be to create a zero dependencies package AccessorsBase that just contains something like

function set end

and ask @andyferris if we can add the Base.setindex! overload at SplitApplyCombine.

Copy link
Member Author

@aplavin aplavin Jun 8, 2022

Choose a reason for hiding this comment

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

Copy link
Member Author

@aplavin aplavin Jun 8, 2022

Choose a reason for hiding this comment

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

Maybe, "pirating" a method that isn't defined, and won't feasibly be defined in the original library is fine? As I understand, the main concern is that piracy can change the behavior of unrelated code - here it's hard to imagine such a case. SAC.jl cannot really define setindex for an arbitrary mappedarray by itself.

Copy link
Member

Choose a reason for hiding this comment

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

That would also be piracy: all these methods, at least https://github.com/JuliaObjects/Accessors.jl/blob/master/src/functionlenses.jl

That is fine by the

Sometimes, coupled packages may engage in type piracy to separate features from definitions

clause in the manual. If we overloaded UnsupectingPackage.set that would be a problem.

Maybe, "pirating" a method that isn't defined, and won't feasibly be defined in the original library is fine?

Fair enough. @andyferris do you think we can do this overload here?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, "pirating" a method that isn't defined, and won't feasibly be defined in the original library is fine?

What can happen is that some other people think the same and provide overloads for Base.setindex!(::MappedArray...) Then a third package might have Accessors and the other package as transitive dependencies and get weird bugs.

Copy link
Member

@rafaqz rafaqz Jun 8, 2022

Choose a reason for hiding this comment

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

Ok I just saw the growing Project.toml. Thats what people will look at to check dependencies.

Mostly everyone is moving away from Requires.jl because its a hack with serious ttfx overheads - it cant precompile.

See the recent breakup of ArrayInterface.jl for a prominent example.

Copy link
Member

Choose a reason for hiding this comment

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

Accessors.jl getting heavy is a good point @rafaqz and getting rid of Requires.jl is also on my wishlist. Currently, we only need it to avoid depending on StaticArrays.jl .
Do you have a suggestion where stuff like this PR could live?

Copy link
Member

@rafaqz rafaqz Jun 8, 2022

Choose a reason for hiding this comment

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

Well we really need conditional dependencies: JuliaLang/Pkg.jl#1285

What ArrayInterface.jl is doing and we're currently doing with GeoInterface.jl is making subpackages in the same repository, named as the combination of packages. Like ArrayInterfaceOffsets.jl or GeoInterfaceTables.jl.

Then other packages and users can choose their dependencies. Its pretty clunky but the best we have currently.

(also sorry I meant to put my initial comment in the main thread I commented from a phone)

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'm also curious how the StaticArrays support in Accessors would look without Requires. Should this https://github.com/JuliaObjects/Accessors.jl/blob/master/src/staticarrays.jl file be put into a separate package that depends on both? The major issue then is that users wouldn't install it and will think that Accessors just doesn't support SVectors.

Copy link
Member

Choose a reason for hiding this comment

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

Yep that's the major unsolved problem that Requires.jl is patching over with a hack: users have to manually import glue packages.

The conditional dependencies issue I linked above is the solution, if it ever gets written.

But we have to be careful stuffing things into Requires.jl, it adds up to a lot of package load time accross the ecosystem. Everything in a require block has to be parsed and precompiled every time we use it, instead of once when packages are updated.

@aplavin aplavin closed this Jul 9, 2022
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