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

feat: refactoring the AwkwardForth code-discovery process #943

Merged
merged 82 commits into from
Dec 5, 2023

Conversation

jpivarski
Copy link
Member

@jpivarski jpivarski commented Aug 17, 2023

tests/test_0637-setup-tests-for-AwkwardForth.py:

  • test_00
  • test_01
  • test_02
  • test_03
  • test_04
  • test_05
  • test_06
  • test_07
  • test_08
  • test_09
  • test_10
  • test_11
  • test_12
  • test_13
  • test_14
  • test_15
  • test_16
  • test_17
  • test_18
  • test_19
  • test_20
  • test_21
  • test_22
  • test_23
  • test_24
  • test_25
  • test_27
  • test_28
  • test_30
  • test_31
  • test_32
  • test_33
  • test_34
  • test_35
  • test_36
  • test_37
  • test_38
  • test_39
  • test_40
  • test_41
  • test_42
  • test_43
  • test_44
  • test_47
  • test_51
  • test_52
  • test_53
  • test_54
  • test_55
  • test_56
  • test_57
  • test_58
  • test_59
  • test_60
  • test_61
  • test_62
  • test_63
  • test_64
  • test_65
  • test_66
  • test_67
  • test_68
  • test_69
  • test_70
  • test_71
  • test_72
  • test_73
  • test_74
  • test_75
  • test_76
  • test_77
  • test_78
  • test_79
  • test_80
  • test_81

tests/test_0798_DAOD_PHYSLITE.py

  • test_AnalysisJetsAuxDyn_GhostTrack
  • test_TruthBosonAuxDyn_childLinks
  • test_TruthPhotonsAuxDyn_parentLinks
  • test_TruthTopAuxDyn_parentLinks

@jpivarski
Copy link
Member Author

This failure

FAILED tests/test_0912-fix-pandas-and-double-nested-vectors-issue-885.py::test_pandas_and_double_nested_vectors_issue_885 - AssertionError: assert False
 +  where False = isinstance(<STLVector [[0.0, 1.0, 2.0], [0.0, 1.0, 2.0, 3.0, 4.0]] at 0x7f3c92b49ac0>, <class 'awkward.highlevel.Array'>)
 +    where <class 'awkward.highlevel.Array'> = <module 'awkward.highlevel' from '/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/awkward/highlevel.py'>.Array
 +      where <module 'awkward.highlevel' from '/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/awkward/highlevel.py'> = ak.highlevel
============ 1 failed, 515 passed, 184 skipped in 94.05s (0:01:34) =============

is because it's expecting AwkwardForth code to run and it isn't. Just insert a test skip, like the others.

@SethBendigo SethBendigo force-pushed the sethbendigo/af-generation-vectorvectordouble branch from 0c7ba7f to 51ce2cb Compare October 26, 2023 05:36
@jpivarski jpivarski mentioned this pull request Nov 10, 2023
@jpivarski
Copy link
Member Author

@SethBendigo, in case you're interested, stack push-pop is a relatively good replacement for UnwindProtect: 222759e. The cost is having to remember to pair up a pop with each push, but the benefit is that the code between the push and pop doesn't have to be duplicated, and we don't have to keep track of a hold_previous_model variable.

@jpivarski
Copy link
Member Author

Look over AsArray code; it has gotten messy

It's fine.

@jpivarski
Copy link
Member Author

The test failures were failing in main, too: https://github.com/scikit-hep/uproot5/actions/runs/7074482533/job/19255559117

I manually triggered the tests on main to check that and forgot to follow up on it.

The test itself was literally saying that this should happen:

# when https://github.com/fsspec/filesystem_spec/pull/1426 is merged this will work, we must remove the catch
with pytest.raises(ValueError):
with uproot.recreate(uri) as f:
f["tree"] = {"x": np.array([1, 2, 3])}
with uproot.open(uri) as f:
assert f["tree"]["x"].array().tolist() == [1, 2, 3]
if scheme == "simplecache::memory://":
raise ValueError("wait for next fsspec release and remove this")

So I'll fix it now, in this PR, and it will carry through to main when I merge it.

@jpivarski
Copy link
Member Author

@lobis, is 05ab1f3 the right way to turn off the "waiting for fsspec/filesystem_spec#1426" error? If not, I'll fix it in this PR. It will enter main when this PR merges (maybe today).

@jpivarski jpivarski marked this pull request as ready for review December 5, 2023 20:38
@lobis
Copy link
Collaborator

lobis commented Dec 5, 2023

@lobis, is 05ab1f3 the right way to turn off the "waiting for fsspec/filesystem_spec#1426" error? If not, I'll fix it in this PR. It will enter main when this PR merges (maybe today).

Yes, this is what was expected.

@jpivarski
Copy link
Member Author

Well, @SethBendigo, it's done! It's finally and fully done. Thank you very much for all of your work on this!!!

When the tests pass one last time, I'll merge it.

@jpivarski jpivarski enabled auto-merge (squash) December 5, 2023 22:23
@jpivarski jpivarski merged commit 8325cb8 into main Dec 5, 2023
20 checks passed
@jpivarski jpivarski deleted the sethbendigo/af-generation-vectorvectordouble branch December 5, 2023 22:33
@agoose77
Copy link
Collaborator

I didn't see that this got in!? How exciting @SethBendigo excellent work, this was a long one.

@SethBendigo
Copy link
Collaborator

Thanks @agoose77! Thanks @jpivarski, I really really appreciated your help throughout. I had a lot of fun and learned a lot!

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.

4 participants