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

Debug for EventwBayesClass because peaks overlapping #1417

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

dachengx
Copy link
Collaborator

Before you submit this PR: make sure to put all operations-related information in a wiki-note, a PR should be about code and is publicly accessible

What does the code in this PR do / what does it improve?

Debug EventwBayesClass. In very rare cases the peaks' "time" can be the same, so simply use the matching between time

mask = np.in1d(peaks["time"], events[f"{name}_time"])
mask_ev = np.in1d(events[f"{name}_time"], peaks["time"])
can cause error like:

CRITICAL:ThreadedMailboxProcessor:Target Mailbox (event_w_bayes_class_paired) killed, exception <class 'strax.mailbox.MailboxKilled'>, message (<class 'ValueError'>, ValueError('NumPy boolean array indexing assignment cannot assign 48003 input values to the 47986 output values where the mask is true'), <traceback object at 0x7f62537a4e00>)

Can you briefly describe how it works?

Mimicking what EventAmbience does, split the peaks for each event and assign the fields based on indices:

def compute(self, events, peaks):
split_peaks = strax.split_by_containment(peaks, events)
# 1. Initialization, ambience is set to be the lowest possible value
result = np.zeros(len(events), self.dtype)
# 2. Assign peaks features to main S1, main S2 in the event
for event_i, (event, sp) in enumerate(zip(events, split_peaks)):
for idx, main_peak in zip([event["s1_index"], event["s2_index"]], ["s1_", "s2_"]):
if idx >= 0:
for ambience in self.origin_dtype:
result[f"{main_peak}n_{ambience}"][event_i] = sp[f"n_{ambience}"][idx]
# 3. Set time and endtime for events
result["time"] = events["time"]
result["endtime"] = strax.endtime(events)
return result
.

The results are not changed so I will not update the __version__.

Can you give a minimal working example (or illustrate with a figure)?

Please include the following if applicable:

  • Update the docstring(s)
  • Update the documentation
  • Tests to check the (new) code is working as desired.
  • Does it solve one of the open issues on github?

Notes on testing

  • Until the automated tests pass, please mark the PR as a draft.
  • On the XENONnT fork we test with database access, on private forks there is no database access for security considerations.

All italic comments can be removed from this template.

@dachengx
Copy link
Collaborator Author

However, this plugin is not widely used.

@dachengx dachengx marked this pull request as ready for review August 27, 2024 06:48
@WenzDaniel
Copy link
Collaborator

I am wondering whether we should not remove this plugin. Is it still being maintained? I do not think so.

@dachengx
Copy link
Collaborator Author

I am wondering whether we should not remove this plugin. Is it still being maintained? I do not think so.

For the reproducibility of old results, we still need it. I think we can keep it for a while.

@WenzDaniel
Copy link
Collaborator

Old results use old container no? Otherwise it is not reproducible anyhow?

@dachengx
Copy link
Collaborator Author

Old results use old container no? Otherwise it is not reproducible anyhow?

Yes. You are right. I mean the reproducibility of the codes in other repo which still uses this plugin.

@coveralls
Copy link

coveralls commented Aug 27, 2024

Coverage Status

Changes unknown
when pulling 166c64c on debug_event_w_bayes_class
into ** on master**.

Copy link
Collaborator

@MerzJohannes MerzJohannes 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. If we want to keep it for reproducibility we can merge it.

Copy link
Collaborator

@yuema137 yuema137 left a comment

Choose a reason for hiding this comment

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

In the previous code, mask may have a larger length than mask_ev (and events) when there are duplicated 'time' in peaks. The PR fixes this problem by looping over split peaks together with events, and only assigning the probability when a valid peak index is found. It looks good to me.

@dachengx dachengx merged commit c1eda3e into master Aug 27, 2024
8 checks passed
@dachengx dachengx deleted the debug_event_w_bayes_class branch August 27, 2024 20:06
dachengx added a commit that referenced this pull request Oct 7, 2024
dachengx added a commit that referenced this pull request Oct 8, 2024
dachengx added a commit that referenced this pull request Nov 16, 2024
* Update pytest.yml (#1431)

* Update pytest.yml

Specify the strax to be v1.6.5

* add install base_env

* Add force reinstall

* Update definition of the SE Score (previously the SE density) for SR1 WIMP (#1430)

* Update score definition. Modify file names.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Modify file names in the initialization file.

* Rearrangenames. Move sr phase assignment elsewhere.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add url configs and modify code style.

* Modify the parameter names.

* Fix data type in url config.

* Add docstring for the eps used to prevent divide by zero.

* Reformmated with precommit.

* Add docstrings. Remove redundant code.

* Add docstring for the 2D Gaussian.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Dacheng Xu <dx2227@columbia.edu>

* Copy #1417 (#1438)

* Bump to v2.2.6 (#1441)

* Bump version: 2.2.4 → 2.2.6

* Update HISTORY.md

* Constraint strax version

---------

Co-authored-by: Kexin Liu <lkx21@mails.tsinghua.edu.cn>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

5 participants