-
Notifications
You must be signed in to change notification settings - Fork 76
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 essential RNTuple writing functionality #1395
Conversation
All the functionality is now ready, I'll do a bit of cleanup tomorrow morning and it's ready to go. |
This is all I was planning to do for now. I ended up adding all the essential functionality that I could think of. There are still Awkward Form/Arrays that are not supported, but I think they are probably not too common and don't have direct translation to RNTuple fields, so I'll deal with them later. You can see from this test data that all essentials should be covered. uproot5/tests/test_1395_rntuple_writing_lists_and_structs.py Lines 16 to 43 in ce96300
It's definitely not perfect, but I'll improve things later. I think it would be good to advertise that we now have experimental writing functionality and see what feedback we start getting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ariostas - Great! It looks good to me. Please, merge if you are done with it.
"jagged_list": [[1], [2, 3], [4, 5, 6]], | ||
"nested_list": [[[1], []], [[2], [3, 3]], [[4, 5, 6]]], | ||
"string": ["one", "two", "three"], | ||
"utf8_string": ["こんにちは", "⚛️💫🎆😀", "ǧ̸̛̫͍̰͖̟̈͛͑͆̆̌̃̉̅̄̔̈́̀̔͆̄͋̍͐͂̎͗̈́͒͘͝ͅö̴̮̝̪̬͎͚̜̖̜͖̞̤͕̙͂̀̀̊͛͑̈́͛͐͊͂͂̇͛̾̔͐͆͑͂̓̅̀͘͘͘̕͝͠͝͝ơ̶͍̙̻̾̈́̓̈́̀̅͑ḑ̷͚̠̹̗͉͙̞͇͕̼̲̥͉̯̞͕̲̻̞͗̓̃̊̅͗͊͊́̑̈́̎͋̇̓͛̅͜͜͠͝ͅb̷̢̢̨̨̛̛̘̠̞̰̺̘̰̖̺̞̱͇̰̙̲̱̪͕͎͉̖̞͇̹̮͙͋̀͑͂̈́̇͛̐͊̀̇͆̓̋̀̿̋̂̅̀̌̑̓̽͊̂͑̈̇̚͜͝y̶̗͇̠̞͚̦̮̦͈̹̥̋̓̓̈́̐̆̀̄̋̂̀̇͋̎̚͜͝ȩ̷̢̡͇̮̩̹̥̬̰͎͔̬̩̰̯͍̲͎̭͉̬̣̻̖͍̥̟̪͕̫̟̋̔̀͆̑̈́̐̃͐͌̍͒̔̈́̃̈́̐̔̾͊̿̓͆͑̚͜͝͝͝ͅ"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow!
|
||
for f in data.fields: | ||
if "tuple" in f: | ||
# TODO: tuples are converted to records |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, this will fail after your PR is merged and released?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let me fix it now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this still works after the other PR since there are some other parts in the RNTuple reading part that need to be updated. I'll work on all those things in a refactoring of the reading part that I'm planning for the next couple of weeks.
This PR extend the writing functionality for RNTuples quite a bit. It adds the following: