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

Dask hists #33

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Dask hists #33

wants to merge 25 commits into from

Conversation

btovar
Copy link
Contributor

@btovar btovar commented Jun 20, 2024

HistEFT to dask

@bryates
Copy link
Contributor

bryates commented Jun 20, 2024

Thanks @btovar! I see a few lint errors
https://github.com/TopEFT/topcoffea/actions/runs/9601776949/job/26481071313?pr=33#step:4:32
Let's see how the main CI runs.

topcoffea/modules/sparseHist.py Outdated Show resolved Hide resolved
topcoffea/modules/sparseHist.py Show resolved Hide resolved
@bryates
Copy link
Contributor

bryates commented Jun 20, 2024

FYI I triggered a CI check in TopEFT using this branch. If that passes, then I'll be happy with merging this PR.

@bryates
Copy link
Contributor

bryates commented Jun 20, 2024

@btovar looks like there's a sqrt error
https://github.com/TopEFT/topeft/actions/runs/9602026457/job/26481917295#step:11:42
Not sure how helpful this will be for debugging, since it's in a function you didn't touch, but it's likely due to a call to eval().

@btovar
Copy link
Contributor Author

btovar commented Jun 20, 2024

There something weird with the tests, though. test_HistEFT_add should be failing...

@Andrew42
Copy link
Contributor

@btovar the CI/CD test that's being done looks to be test_HistEFT_add, but should that instead be test_histEFT_add?

@bryates
Copy link
Contributor

bryates commented Jun 20, 2024

@btovar the CI/CD test that's being done looks to be test_HistEFT_add, but should that instead be test_histEFT_add?

Good catch! I just pushed that. Looks like the unit test is still for HistEFT.

@kmohrman
Copy link
Contributor

Looks like the CI is still failing. Seems to be a ModuleNotFoundError for dask.

Copy link
Contributor

@bryates bryates left a comment

Choose a reason for hiding this comment

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

Thanks @btovar, I think this looks good.


def _fill_indices(self, n_events):
# turns [0, 1, 2, ..., num of quadratic coeffs - 1]
# into:
# [0, 1, 2, ..., 0, 1, 2 ...,]
# repeated n_events times.
return self.array_backend.broadcast_to(
np.ogrid[0 : self.quad_count], (n_events, self.quad_count)
np.ogrid[0 : self.quad_count], (n_events, self.quad_count),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the comma needed? I guess it doesn't hurt anything.

@bryates
Copy link
Contributor

bryates commented Jun 21, 2024

Needed because coffea is pinned to an older version
@btovar
Copy link
Contributor Author

btovar commented Jun 21, 2024

Ah yes, the old coffea does not add them.

@bryates
Copy link
Contributor

bryates commented Jun 21, 2024

Ah yes, the old coffea does not add them.

Ok, now it's an issue with the label keyword

E   TypeError: __init__() got an unexpected keyword argument 'label'

@bryates
Copy link
Contributor

bryates commented Jun 24, 2024

@btovar looks like the current CI failure might just be a precision issue
https://github.com/TopEFT/topcoffea/actions/runs/9649881600/job/26614283607?pr=33#step:11:32
It's just slightly above 1e-10

@bryates
Copy link
Contributor

bryates commented Jun 24, 2024

Thanks @btovar! Looks like the CI here is passing now. I've restarted the CI on TopEFT to make sure it works there as well.

@btovar
Copy link
Contributor Author

btovar commented Jun 24, 2024

Nice! I need to update topEFT, though, and probably propagate changes to this pr.

@bryates
Copy link
Contributor

bryates commented Jun 24, 2024

Nice! I need to update topEFT, though, and probably propagate changes to this pr.

Nice! I need to update topEFT, though, and probably propagate changes to this pr.

Nice! I need to update topEFT, though, and probably propagate changes to this pr.

Ok thanks. It's complaining about wc_names
https://github.com/TopEFT/topeft/actions/runs/9650133511/job/26615134823#step:12:143
Is that related to what you need to change?

@btovar
Copy link
Contributor Author

btovar commented Jun 24, 2024

I think so. For the new hists the axes need to be separated into lists of category or dense so that we know which axes are available at dask graph construction time.

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