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: Form.expected_from_buffers for names/dtypes ak.from_buffers needs. #2660

Merged
merged 3 commits into from
Aug 23, 2023

Conversation

jpivarski
Copy link
Member

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #2660 (954140d) into main (0accfad) will increase coverage by 0.04%.
The diff coverage is 92.18%.

Additional details and impacted files
Files Changed Coverage Δ
src/awkward/forms/unionform.py 86.25% <66.66%> (-1.83%) ⬇️
src/awkward/forms/form.py 81.68% <68.42%> (-0.92%) ⬇️
src/awkward/contents/emptyarray.py 75.36% <100.00%> (ø)
src/awkward/forms/bitmaskedform.py 83.78% <100.00%> (+1.25%) ⬆️
src/awkward/forms/bytemaskedform.py 85.71% <100.00%> (+1.17%) ⬆️
src/awkward/forms/emptyform.py 86.86% <100.00%> (+0.84%) ⬆️
src/awkward/forms/indexedform.py 80.00% <100.00%> (+1.49%) ⬆️
src/awkward/forms/indexedoptionform.py 89.62% <100.00%> (+0.84%) ⬆️
src/awkward/forms/listform.py 78.94% <100.00%> (+1.80%) ⬆️
src/awkward/forms/listoffsetform.py 94.28% <100.00%> (+0.40%) ⬆️
... and 5 more

@jpivarski jpivarski temporarily deployed to docs-preview August 18, 2023 19:37 — with GitHub Actions Inactive
@agoose77
Copy link
Collaborator

agoose77 commented Aug 21, 2023

This looks pretty good!

I'm not yet sure whether we want this logic to be implemented by the Form classes, though. My thinking is thus;

  • We want to separate the concerns of Form and Content.
  • Content handles to_buffers, and a top-level routine handles from_buffers

As such, we should probably have a top-level routine that handles expected_from_buffers.

I'm also thinking about whether Form.expected_from_buffers should instead be accessed via ak.expected_from_buffers. We do have an array-aware method on Form: Form.length_zero_array, but that perhaps should be the exception rather than the rule.

I can make these changes — at this stage it's just musings.

@jpivarski
Copy link
Member Author

This kind of functionality should be level-2: public but not widely advertised. I don't think it should be a high-level ak.* function, for instance.

It could be a free function in the ak.forms namespace, like ak.forms.from_dict and ak.forms.from_json. Nothing in the name expected_from_buffers says that it's about Forms, rather than layouts, so

ak.forms.expected_from_buffers(some_form)

would accomplish that as well as

some_form.expected_from_buffers

I'm not actually in favor of the following, but just to consider all options:

ak.from_buffers.expected(some_form)

@agoose77
Copy link
Collaborator

I don't think it should be a high-level ak.* function, for instance.

There're probably too many constraints here to make everything work. I've changed my mind; although from_buffers is written in the ak_ namespace, there's nothing to suggest that we can't encode a canonical form representation in ak.forms, which is what this does. I'm +1, although I'd therefore like to move the functions that are imported from ak_from_buffers to ak.forms itself (my todo).

@agoose77
Copy link
Collaborator

@jpivarski could you OK (nOK) my counter edits?

@agoose77 agoose77 temporarily deployed to docs-preview August 21, 2023 18:42 — with GitHub Actions Inactive
@agoose77 agoose77 changed the title feat: 'Form.expected_from_buffers' for names/dtypes 'ak.from_buffers' needs. feat: Form.expected_from_buffers for names/dtypes ak.from_buffers needs. Aug 21, 2023
@jpivarski
Copy link
Member Author

@jpivarski could you OK (nOK) my counter edits?

Okay!

@jpivarski jpivarski enabled auto-merge (squash) August 23, 2023 14:28
@jpivarski jpivarski temporarily deployed to docs-preview August 23, 2023 14:41 — with GitHub Actions Inactive
@jpivarski jpivarski merged commit 7a6b253 into main Aug 23, 2023
34 checks passed
@jpivarski jpivarski deleted the jpivarski/expected-container-keys-from-form branch August 23, 2023 14:54
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.

None yet

2 participants