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

Add support for reading TDatime #407

Merged
merged 6 commits into from
Aug 19, 2021
Merged

Add support for reading TDatime #407

merged 6 commits into from
Aug 19, 2021

Conversation

veprbl
Copy link
Contributor

@veprbl veprbl commented Aug 7, 2021

No description provided.

@jpivarski
Copy link
Member

Should this be linked in with

https://github.com/scikit-hep/uproot4/blob/c1c95c0ad6b6f1fe830ed7187a71420d5fff913b/src/uproot/_util.py#L814-L839

which convert ROOT's "datime" integers into Python datetime objects?

@veprbl
Copy link
Contributor Author

veprbl commented Aug 9, 2021

Should this be linked in with

For sure, but in what way? Modify during read, or in Model.postprocess or add as a method?

@jpivarski
Copy link
Member

For sure, but in what way? Modify during read, or in Model.postprocess or add as a method?

The canonical way would be to add a src/behaviors/TDatime.py containing a TDatime class and have this Model_TDatime inherit from that behavior. The behavior would then define a

    def to_datetime(self):
        return uproot._util.code_to_datetime(self._members["fDatime"])

method for Model_TDatime to inherit.

That's how the methods are usually named: histograms have to_numpy(), to_hist() (methods, not properties, to allow for future parameterization). The goal is not to rewrite the functionality of ROOT in Uproot, but to export objects to their Python equivalents (which generally provide the functionality of ROOT by themselves; for instance, datetime has all the methods for getting year, day, hours, isoformat, etc.).

jpivarski pushed a commit to scikit-hep/scikit-hep-testdata that referenced this pull request Aug 9, 2021
* Add uproot-issue407.root

* uproot-issue407.root -> uproot-issue-407.root, fix macro name

* restore uproot-issue407.root
@jpivarski
Copy link
Member

scikit-hep-testdata 0.4.6 is now available. Tests that are looking for uproot-issue-407.root should find it.

@veprbl
Copy link
Contributor Author

veprbl commented Aug 18, 2021

Could someone trigger the tests? I believe this is ready.

@jpivarski
Copy link
Member

Sorry, I didn't realize that it was being held up.

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.

Looks good to me! I'll set it to merge when the tests pass.

@jpivarski jpivarski enabled auto-merge (squash) August 18, 2021 16:15
@jpivarski jpivarski merged commit caeaca9 into scikit-hep:main Aug 19, 2021
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