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

feat: Drop ak.behavior['.', 'Name'] = cls, which isn't working/isn't tested. #1651

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

jpivarski
Copy link
Member

This feature had only one test in v1 and was untested in v2. It was also implemented incorrectly (layout.parameter("__record__") where layout is a list wouldn't see the record's parameter; you'd need a variant of purelist_parameter that only goes one level deep).

But it seems like this is unwanted: I've never heard of anybody using this feature, not Vector and not Coffea, which are the two heaviest users of behavioral overrides. (Early on, I was thinking of regular expressions: . for a single character and * for a sequence. However, we now have a lot more experience with how these things are used in the wild.)

I'm making this its own pull request so that it will be highly visible, and we can consider reverting it, though we'd have to do more than that: we'd have to make it not-broken.

One thing that should go with this: references to

ak.behavior[".", "Name"] = Class

need to be removed from any forward-looking (v2) documentation.

@agoose77
Copy link
Collaborator

agoose77 commented Aug 30, 2022

We spoke about this in general before, with an eye towards extending the syntax to permit users to specify dimension-aware behaviours. I wanted this at one point to better enforce structural constraints for complex behaviour classes.

I rgd my repos, and found ak.behavior[(".", "ordered_map")] = OrderedMapArray, which is an implementation of a key-value abstraction. So, I can attest to at least one usage ;)

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #1651 (166960e) into main (9e17f29) will increase coverage by 0.54%.
The diff coverage is 67.67%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_v2/_connect/cuda/__init__.py 0.00% <0.00%> (ø)
src/awkward/_v2/_connect/numexpr.py 88.40% <0.00%> (ø)
src/awkward/_v2/_connect/pyarrow.py 88.46% <0.00%> (ø)
...awkward/_v2/_connect/rdataframe/from_rdataframe.py 0.00% <0.00%> (ø)
src/awkward/_v2/_lookup.py 98.68% <ø> (+1.17%) ⬆️
src/awkward/_v2/numba.py 93.47% <0.00%> (ø)
src/awkward/_v2/operations/ak_from_avro_file.py 66.66% <0.00%> (ø)
src/awkward/_v2/operations/ak_local_index.py 100.00% <ø> (ø)
src/awkward/_v2/operations/ak_max.py 87.09% <ø> (ø)
src/awkward/_v2/operations/ak_min.py 87.09% <ø> (ø)
... and 59 more

@jpivarski
Copy link
Member Author

I remember that. If dropping the "." case makes it easier to extend the syntax, then that's an additional benefit. What we can't change, however, is the "*" case, since a lot of people know about it and use it.

So, I can attest to at least one usage ;)

In that case, do you need it? (Can your ordered map function without it?) We can reinstate this feature. As I said in Slack, it will be easier to remove it and introduce it correctly than to leave it as-is.

@agoose77
Copy link
Collaborator

I found the relevant issue: #1038

In that case, do you need it? (Can your ordered map function without it?)

Not urgently - I just lose some safety. I'm not convinced it's enough to arrest this PR, though :) I'm not being contrary, though it looks like it, just setting out my own usage before we merge this :)

@agoose77 agoose77 changed the title feat: Drop ak.behavior['.', 'Name'] = Class, which isn't working/isn't tested. feat: Drop ak.behavior['.', 'Name'] = cls, which isn't working/isn't tested. Aug 30, 2022
@jpivarski
Copy link
Member Author

I'm going to merge this with the understanding that reintroducing the "." syntax is definitely on the table. But it's something we'll proactively do, rather than let that wrong code sit there untested.

@jpivarski jpivarski merged commit dd2a3f4 into main Aug 30, 2022
@jpivarski jpivarski deleted the jpivarski/drop-single-dot-behavior-syntax branch August 30, 2022 22:07
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