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

Fix bug when processing files with a single event #393

Merged
merged 6 commits into from
Apr 28, 2023

Conversation

nkx111
Copy link
Member

@nkx111 nkx111 commented Apr 18, 2023

nkx111 Ok: 40

Issue #390.
The bug comes from when finished doing test run(before the actual event processing), REST will push-forward to the next event. Since there is only one event in the file, find the next non-existing event will actually trigger finish reading. Therefore the later processes will get null input event, and cause problems. This fix it to prevent push-forward of event entries during test run, if test run succeeds.

@nkx111 nkx111 changed the title TRestThread test run reduces call for GetNextEvent Fix bug when processing files with a single event Apr 18, 2023
@nkx111 nkx111 marked this pull request as ready for review April 18, 2023 14:45
@nkx111 nkx111 requested review from jgalan and lobis April 18, 2023 14:46
@jgalan
Copy link
Member

jgalan commented Apr 18, 2023

Nice! I guess some pipelines are failing because it is not the same event that it is being evaluated?

@juanangp
Copy link
Member

Nice! I guess some pipelines are failing because it is not the same event that it is being evaluated?

Still don't understand why some pipelines are failing, if you are checking a different event why only one observable has a different value e.g. on the TREX-DM validation pipeline? I would expect several observables failing...

@jgalan
Copy link
Member

jgalan commented Apr 20, 2023

Nice! I guess some pipelines are failing because it is not the same event that it is being evaluated?

Still don't understand why some pipelines are failing, if you are checking a different event why only one observable has a different value e.g. on the TREX-DM validation pipeline? I would expect several observables failing...

The observable that it is failing is rate_MeanRate_InHz this observable does not only depend on the specific entry being evaluated but it takes the timestamp value of a different event (shifted by 10 events, for example) to calculate the mean value. If there is an extra event ahead this value could be affected. This is a guess.

For the ray-tracing case, all the values are changing, so I guess it is an issue affecting the seed by the fact that there is a different looping scheme? what do you think @nkx111 ?

Merging the following PR should fix axion ray-tracer pipeline rest-for-physics/axionlib#55

@juanangp
Copy link
Member

rate_MeanRate_InHz

Then this observable doesn't make much sense to me, in general we should have one observable per event, the rate can be computed later on...

@nkx111
Copy link
Member Author

nkx111 commented Apr 20, 2023

The observable that it is failing is rate_MeanRate_InHz

I looked into each timestamp of the first 10 events from the pipeline. It changes after this PR. I suspect that it is related somehow with the bug in TRestRawMultiFEMINOSToSignalProcess we already recorded. rest-for-physics/rawlib#36

Before the PR:

$test run: 1582141590.00456

$event 0: 1582141590.00629
$event 1: 1582141590.00456
$event 2: 1582141590.00629
$event 3: 1582141590.00627
$event 4: 1582141590.0068
$event 5: 1582141590.00771
$event 6: 1582141590.00961
...

With this PR:

$test run: 1582141590.00456

$event 0: 1582141590.00456
$event 1: 1582141590.00456
$event 2: 1582141590.00629
$event 3: 1582141590.00627
$event 4: 1582141590.0068
$event 5: 1582141590.00771
$event 6: 1582141590.00961

The assumed "correct behavior", considering the TRestRawMultiFEMINOSToSignalProcess issue, is that the first two events are identical. This is exactly the case with this PR. Before this PR, however, with additional "get-next-event" operation called after test run, event id 0 mysteriously turns into event id 2. The cause of it is unknown to me.

Anyway, despite the first few values of rate_MeanRate_InHz, this observable is correct and meaningful. I think we can just update the checks. In future when we fix the bug in TRestRawMultiFEMINOSToSignalProcess, the validation file shall be updated again. I am not familiar with the FEMINOS daq system, so someone else could help to fix that process?

@nkx111
Copy link
Member Author

nkx111 commented Apr 20, 2023

For the ray-tracing case, all the values are changing, so I guess it is an issue affecting the seed by the fact that there is a different looping scheme?

Yes, I think this is the case. Previously we are calling TRestAxionGeneratorProcess::ProcessEvent() once more than this PR. So the random value gets changed.

@jgalan
Copy link
Member

jgalan commented Apr 26, 2023

rate_MeanRate_InHz

Then this observable doesn't make much sense to me, in general we should have one observable per event, the rate can be computed later on...

Yes, that might be a good choice. But does that mean that we need to move TRestEventRateAnalysisProcess to legacy?

@jgalan
Copy link
Member

jgalan commented Apr 26, 2023

the validation file shall be updated again. I am not familiar with the FEMINOS daq system, so someone else could help to fix that process?

Ok, I think it is clear that it is harmless. I can update the file.

@juanangp Thinking a bit more about, it is interesting to maintain the TRestEventRateAnalysisProcess because it can capture the mean rate at different stages of the processing, while this we cannot do with the final analysis.

For example, in the case that at the raw stage we plug in the TRestEventRateAnalysisProcess it will capture a given mean rate, however, if by some means, the number of events gets reduced when we reach the detector or track domain, then we can plug the TRestEventRateAnalysisProcess there and get a different value. Of course, this process can be also plugged at different raw processing stages where the mean rate is different before or after event discrimination. This is possible using the <cut definition.

@jgalan jgalan merged commit 324cf3d into master Apr 28, 2023
@jgalan jgalan deleted the nkx111-single-event-file branch April 28, 2023 14:13
jgalan added a commit that referenced this pull request May 3, 2023
…vent-file"

This reverts commit 324cf3d, reversing
changes made to babeff3.
jgalan added a commit that referenced this pull request May 3, 2023
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.

Bug when processing files with a single event
3 participants