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: All of the Awkward layout types. #13

Merged
merged 22 commits into from
Aug 26, 2023

Conversation

jpivarski
Copy link
Member

@jpivarski jpivarski commented Aug 22, 2023

  • PrimitiveArray (is LeafType)
  • EmptyArray (is LeafType)
  • ListOffsetArray (is ListType)
  • ListArray (is ListType)
  • RegularArray (is ListType)
  • IndexedArray
  • IndexedOptionArray (is OptionType)
  • ByteMaskedArray (is OptionType)
  • BitMaskedArray (is OptionType)
  • UnmaskedArray (is OptionType)
  • RecordArray and AwkwardArray.Record
  • TupleArray and AwkwardArray.Tuple
  • UnionArray

) where {INDEX<:IndexBig} where {CONTENT<:Content} =
ListArray(INDEX([]), INDEX([]), CONTENT(), parameters = parameters, behavior = behavior)

function copy(
Copy link
Member

Choose a reason for hiding this comment

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

is this suppose to be Base.copy? I guess there's also Base.deepcopy() so it's one of them probably

Copy link
Member Author

Choose a reason for hiding this comment

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

It's supposed to be distinct from Base.copy. Layout nodes in Awkward each have a constructor named copy that shallow-copies every argument that is not named: it's a great way to get "a struct exactly like this_one but with this_field and that_field replaced."

Before the final version of this package, the name might change to something that doesn't sound like it's supposed to be Base.copy. Or if there's a Julia way of doing this, it would be replaced by that. (Note that this copy can change type parameters as well as values.)

@Moelf
Copy link
Member

Moelf commented Aug 22, 2023

Is there a conceptual or application-wise difference between Unset and built-in singleton type nothing::Nothing?

@jpivarski
Copy link
Member Author

In Awkward, IndexedOptionArray satisfies both is_indexed and is_option, which I'm implementing here with abstract types (isa-checkable interfaces; no data or constructors). But it seems that multiple inheritance of abstract types is not yet a feature of the language (JuliaLang/julia#5).

So I'll just ignore is_indexed and only implement is_option, so that the inheritance is strictly a tree. It's a minor point; just noting this for the record.

@jpivarski
Copy link
Member Author

Is there a conceptual or application-wise difference between Unset and built-in singleton type nothing::Nothing?

Some of the arguments for copy could legitimately be nothing::Nothing. Let me think of an example... Actually, no! RecordArray.fields would have been the example (in Python, fields=None means it's a tuple with unnamed fields, rather than a record with named fields), but in Julia, I'm using a NamedTuple to provide both field names and contents. On the one hand, that means that there happen to be no Content subclass arguments that could take a None/nothing value, and on the other hand, it means that I'm not done with RecordArray because I'll need to implement tuples as a separate class, based on Tuple, rather than NamedTuple.

But even if I switch all of the Unset sentinels over to Nothing, there's a (small) chance that in the future, a parameter will be added that can legitimately be nothing, and then it would be hard to switch all the Nothings back into Unset. It would break backward compatibility.

The reason is completely analogous to why these methods in Python have UNSET defaults, rather than None:

https://github.com/scikit-hep/awkward/blob/0accfadd25d6562813e91771177bb5b0bda5a7b1/src/awkward/contents/indexedarray.py#L135-L140

src/AwkwardArray.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
@jpivarski jpivarski linked an issue Aug 23, 2023 that may be closed by this pull request
test/runtests.jl Outdated
Comment on lines 982 to 988
# this is the tricky part: users have to add dummy values to records
# with Bit/ByteMaskedArray and *not* with IndexedOptionArray
AwkwardArray.push!(a_layout, 7)
AwkwardArray.push!(b_layout, 7.7)
AwkwardArray.push_null!(layout)
@test length(layout) == 7
@test ismissing(layout[7])
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: if all Content subclasses implement zero, then it would be possible to auto-fill records and not require users to do something different when push_null! on option-type of record. That would be a good thing to do and this should become an issue.

@jpivarski jpivarski marked this pull request as ready for review August 26, 2023 00:29
@jpivarski jpivarski enabled auto-merge (squash) August 26, 2023 00:30
@jpivarski jpivarski merged commit fd5113f into main Aug 26, 2023
2 checks passed
@jpivarski jpivarski deleted the jpivarski/all-the-layout-types branch August 26, 2023 00:31
@Moelf
Copy link
Member

Moelf commented Aug 26, 2023

Congrats!! that was unbelievably fast

@jpivarski
Copy link
Member Author

Thanks!

Some things will be easier to apply, now that all of the node types exist.

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.

All of the Awkward layout types.
2 participants