fix: Raise error when converting xml workspaces without data

[alexander-held]

Description

Raise an error when attempting to convert an xml workspace that contains no data, i.e. looks like this:

<Data HistoName="" InputFile="" HistoPath=""/>

Previously this would raise an IsADirectoryError via uproot, which is confusing to users. See #566 for context.

I have not added any new test for this, as I haven't seen anything specifically targeting that API at the moment. I confirmed it raises as expected, an example is linked in #566 (comment).

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

* Raise NotImplementedError when attempting to convert an xml workspace that
  contains no data. The previous behavior was uproot raising IsADirectoryError,
  which is confusing.
* Add a test for raising NotImplementedError in tests/test_import.py and add the
  test files.

[alexander-held]

I realized that this affects lots of tests that probably relied on the function just doing nothing, so I'll look at a better way to implement.

edit: I forgot that histopath can legitimately be an empty string, fixed now.

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (c3a00e8) 98.30% compared to head (34e0b7b) 98.30%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2109   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          69       69           
  Lines        4531     4533    +2     
  Branches      800      801    +1     
=======================================
+ Hits         4454     4456    +2     
  Misses         45       45           
  Partials       32       32           

Flag	Coverage Δ
contrib	97.88% <100.00%> (+<0.01%)	⬆️
doctest	61.12% <0.00%> (-0.03%)	⬇️
unittests-3.10	96.31% <100.00%> (+<0.01%)	⬆️
unittests-3.11	96.31% <100.00%> (+<0.01%)	⬆️
unittests-3.8	96.33% <100.00%> (+<0.01%)	⬆️
unittests-3.9	96.36% <100.00%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
src/pyhf/readxml.py	96.41% <100.00%> (+0.03%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[matthewfeickert]

Thanks @alexander-held. We (someone, doesn't have to be you) should add a test that covers this use case so that we don't lose coverage, but overall this looks good. 👍

[alexander-held]

Do you just want a unit test or something more integration-like? I didn't find any related tests for this part actually, so I was not sure where to best add something.

[matthewfeickert]

Just a unit test that covers the raise NotImplementedError line which codecov says isn't' fully covered at the moment.

[alexander-held]

While looking where to put a test I came across test_import_noChannelData and #1911 / #1931 (cc @kratsg) which I had completely forgotten about. Not sure yet why this did not catch my example, will have a look.

edit: The difference is that the old PR is for the case where the data field is missing completely, while the case I am used to has a data field but the paths are empty strings.

[matthewfeickert]

@alexander-held sorry this hasn't been merged yet. It is late my time in Canada right now, but I have this on my list of things to do tomorrow during conference breaks.

[matthewfeickert]

LGTM. Thanks @alexander-held and sorry about the months of delay.