Skip to content

Conversation

@aplavin
Copy link
Collaborator

@aplavin aplavin commented Oct 25, 2021

There is no ambiguity in the first mapview() argument anyway.
This makes it possible to use any callable (not covered by Base.Callable) as f in mapview - for example, @optic(...) from Accessors.jl.

@andyferris
Copy link
Member

OK, interesting. Thanks @aplavin.

This is possibly fine here but elsewhere the API is a bit strange and you need to disambiguate which terms are collections and which are functions. I don't really like using Base.Callable in these cases but its a necessary evil and hard to work around given how similar we like to keep the API to Base, etc, although at some point we should just go ahead and make breaking changes and split functions up a bit.

(Another thing which would be nice with map etc is if they could be curried, but zero-collection map is defined in Base as a kind of 0-dimensional style of generalization, so I'd like to consider also how this fits together).

PS I have created an issue in Accessors.jl to see if the lenses etc could subtype Function.

@aplavin
Copy link
Collaborator Author

aplavin commented Oct 26, 2021

As I understand, the major ambiguity is in the group function - its first argument can either be a callable or an array. This PR is focused on mapview alone, so there shouldn't be any such issues.

Actually, I encountered this incompatibility (Accessors.jl lenses vs mapview) when trying to make mapview result support setindex!. It turned out to be surprisingly easy: the only thing needed in addition to relaxed type constraints (remove Base.Callable - this PR) is this method:

Base.setindex!(ma::SplitApplyCombine.MappedArray, value, ix) = ma.parent[ix] = Accessors.set(ma.parent[ix], ma.f, value)

Then assigning to mapview elements just works:
image
Of course, this method cannot really be put into either of these packages because they don't depend on each other, so currently I use it in a type-piratic way.

Copy link
Member

@andyferris andyferris left a comment

Choose a reason for hiding this comment

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

OK we should probably merge this one

@andyferris andyferris merged commit afd21e8 into JuliaData:main Nov 9, 2021
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