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

build: Begin move to uproot4 #1002

Merged
merged 27 commits into from
Jan 5, 2021
Merged

build: Begin move to uproot4 #1002

merged 27 commits into from
Jan 5, 2021

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Jul 28, 2020

Description

Following PR #930 and the release of v0.5.0, this PR now moves to adopting uproot4 and its API.

As of uproot4 v4.0.0 uproot4 does not support the ability to write to ROOT files, so uproot3 must still be retained to write to ROOT. This is the only remaining requirement for uproot3 and so the move to the uproot4 API can begin now.

As uproot4 is also added to the HEAD of dependnecies workflow, the pattern of

import uproot

is used instead of

import uproot4 as uproot

as installing uproot4 from GitHub installs under the uproot namespace. So to test the HEAD of uproot4, the uproot namespace must be used now instead of waiting to drop uproot3.

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Require uproot4 in the xmlio extra
   - Require release compatible with uproot v4.X series 
* Adopt the uproot4 v4.0 API
   - uproot3 still needed for writing ROOT files
* Add uproot4 to the HEAD of dependencies workflow
   - import uproot namespace everywhere uproot4 is used to allow for testing of uproot4 HEAD

@matthewfeickert matthewfeickert added refactor A code change that neither fixes a bug nor adds a feature build Changes that affect the build system or external dependencies labels Jul 28, 2020
@matthewfeickert matthewfeickert self-assigned this Jul 28, 2020
src/pyhf/readxml.py Outdated Show resolved Hide resolved
src/pyhf/writexml.py Outdated Show resolved Hide resolved
src/pyhf/readxml.py Outdated Show resolved Hide resolved
@matthewfeickert matthewfeickert force-pushed the build/use-uproot4 branch 2 times, most recently from d4f6bfd to a2ac9e8 Compare September 8, 2020 23:27
@matthewfeickert matthewfeickert force-pushed the build/use-uproot4 branch 3 times, most recently from 492f872 to 9c1e7d8 Compare November 24, 2020 15:22
@matthewfeickert matthewfeickert changed the title build: Move to uproot4 build: Begin move to uproot4 Dec 24, 2020
@codecov
Copy link

codecov bot commented Dec 24, 2020

Codecov Report

Merging #1002 (738d480) into master (5931c16) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1002   +/-   ##
=======================================
  Coverage   97.46%   97.47%           
=======================================
  Files          63       63           
  Lines        3713     3716    +3     
  Branches      524      525    +1     
=======================================
+ Hits         3619     3622    +3     
  Misses         55       55           
  Partials       39       39           
Flag Coverage Δ
unittests 97.47% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pyhf/cli/rootio.py 92.15% <100.00%> (ø)
src/pyhf/readxml.py 94.33% <100.00%> (+0.03%) ⬆️
src/pyhf/writexml.py 95.83% <100.00%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5931c16...738d480. Read the comment docs.

@matthewfeickert matthewfeickert marked this pull request as ready for review December 24, 2020 09:39
src/pyhf/readxml.py Outdated Show resolved Hide resolved
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.

Just some comments inline!

.github/workflows/dependencies-head.yml Show resolved Hide resolved
src/pyhf/readxml.py Outdated Show resolved Hide resolved
src/pyhf/readxml.py Outdated Show resolved Hide resolved
@kratsg
Copy link
Contributor

kratsg commented Jan 4, 2021

One thing, can you use uproot4's sphinx referencing?

intersphinx_mapping = {
    'uproot': ('https://uproot.readthedocs.io/en/latest/', None),
}

which should work pretty well in our case (unless we need to point to a specific version).

@kratsg kratsg force-pushed the build/use-uproot4 branch from bfe4e5e to 538265a Compare January 5, 2021 13:47
@kratsg kratsg mentioned this pull request Jan 5, 2021
4 tasks
@kratsg kratsg merged commit e3da2dc into master Jan 5, 2021
@kratsg kratsg deleted the build/use-uproot4 branch January 5, 2021 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Changes that affect the build system or external dependencies refactor A code change that neither fixes a bug nor adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants