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

Adding a AwkwardForth based ROOT reader to uproot4 #610

Closed
wants to merge 25 commits into from

Conversation

aryan26roy
Copy link
Collaborator

No description provided.

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

I'm not going to merge this now; it involves a lot of changes to the read methods. There are a lot of read methods in the Uproot codebase—adding all of this to each one of them would add a lot of complexity.

There should be a way to generalize what you've done with these two read methods (on AsVector and AsString) so that it's less invasive. Somehow, the code that performs this logic should be mostly outside of read, maybe something that's just called.

"""


class forth_obj:
Copy link
Member

Choose a reason for hiding this comment

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

PEP8 class names: forth_objForthObj.


class forth_obj:
"""
Util class for holding information related to Forth code generation.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Util class for holding information related to Forth code generation.
This class is passed through the Forth code generation, collecting Forth snippets and concatenating them at the end.

Comment on lines 77 to 80


class _PreReadDoneError(Exception):
pass
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class _PreReadDoneError(Exception):
pass

Copy link
Member

Choose a reason for hiding this comment

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

Because it's no longer used.

@@ -11,10 +11,12 @@
import struct
import types
from collections.abc import KeysView, Mapping, Sequence, Set, ValuesView
from re import L # noqa : F401
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from re import L # noqa : F401

Not used.


import numpy

import uproot
from uproot.deserialization import DeserializationError # noqa : F401
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from uproot.deserialization import DeserializationError # noqa : F401

Comment on lines 411 to 412
# def descent(self, context):

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# def descent(self, context):

Don't leave in commented-out code.

Comment on lines 467 to 470
isinstance(
forth_obj.aform,
(awkward.layout.NumpyArray,),
)
Copy link
Member

Choose a reason for hiding this comment

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

This code doesn't do anything.

Also, if aform is really a Form, it can never have type NumpyArray.

@jpivarski
Copy link
Member

@all-contributors please add @aryan26roy for code

@allcontributors
Copy link
Contributor

@jpivarski

I've put up a pull request to add @aryan26roy! 🎉

@jpivarski
Copy link
Member

Is this superseded by #644? If so, let's close the old ones so that I don't get confused about which one to look at.

@jpivarski jpivarski closed this Jul 19, 2022
@jpivarski jpivarski deleted the aryan-forth-reader branch July 19, 2022 18:51
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