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.

Sign up for GitHub

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

Jump to bottom

Duplicate entries if FairVolumeList #1595

Open
TobiasStockmanns opened this issue Nov 11, 2024 · 8 comments
Open

Duplicate entries if FairVolumeList #1595

TobiasStockmanns opened this issue Nov 11, 2024 · 8 comments
Assignees
@karabowi

Comments

@TobiasStockmanns
Copy link
Contributor

Describe the bug
When running a simulation macro with PandaRoot a number of errors is created, all of the type:

[ERROR] FairVolumeList element: PixelActiveo5 VolId : 6032 already defined 6030
[ERROR] FairVolumeList element: PixelActiveo5oPartAss VolId : 6032 already defined 6031
[ERROR] FairVolumeList element: PixelActiveo5 VolId : 6032 already defined 6030
[ERROR] FairVolumeList element: PixelActiveo5oPartAss VolId : 6032 already defined 6031

The error shows up in the recent dev branch of FairRoot and is not present in v18.8.2

To Reproduce
Steps to reproduce the behavior:

  1. FairSoft dev, FairRoot dev
  2. PandaRoot dev
  3. run /macro/master/sim_complete.C
@karabowi karabowi self-assigned this Nov 19, 2024
karabowi added a commit to karabowi/FairRoot that referenced this issue Nov 22, 2024
@karabowi
fix(sim): Check for multiple SensitiveVolumes with same name
6c0e792 
Both the transport engines and FairRoot allows for registering
multiple nodes with the same volume/same volume name (copy mechanism).
However currently such workflow logs errors in `FairVolumeList::addVolume()`.

The commit reintroduces the check for same volume names in the
`FairModule::AddSensitiveVolume()` function thus preventing
registration of copy volumes.

Fixes the issue FairRootGroup#1595.
karabowi added a commit to karabowi/FairRoot that referenced this issue Nov 22, 2024
@karabowi
fix(sim): Add check for multiple sensitive volumes with same name
e351b22 
Both the transport engines and FairRoot allow registering
of multiple nodes with the same volume/same volume name (copy mechanism).
However currently such workflow logs errors from `FairVolumeList::addVolume()`.

The commit reintroduces the check for same volume names into the
`FairModule::AddSensitiveVolume()` function thus preventing
registration of copy volumes.

Fixes the issue FairRootGroup#1595.
@karabowi karabowi mentioned this issue Nov 22, 2024
Open
1 task
@dennisklein
Copy link
Member

dennisklein commented Nov 22, 2024 •
edited
Loading

This is a consequence of fixing #1495, which removed duplicate searches. Now, the only check happens inside addVolume - which currently decides to print an error (the one you see). IMHO, we should just remove this error log from addVolume as it now communicates back to the caller whether the volume was added or not (f852585).

@ChristianTackeGSI
Copy link
Member

First: I agree with Dennis.

Note: I think, we added the error message there, because the new implementation effectively (using unique_ptr) deletes the passed in FairVolume. So this was some sort of "You gave me a FairVolume, well, I deleted it. Maybe that's a real problem?".

For me it is still not clear, whether this is "expected" behaviour? If so, IMHO the caller that expects this behaviour should at least (using comments) document that this is "okay".

All that said:

  1. Is this error happening anywhere in the FairRoot "testsuite" (vulgo examples)?

    If yes: Good, then we have some "expected" happenings and should address them (by documentation).

    If not: See next point

  2. (If not) What is PandaRoot performing differently? If PandaRoot's usage of FairRoot is fine, then we maybe should add another example that actually utilized this behaviour?

@dennisklein
Copy link
Member

we added the error message

If you mean recently, no, we did not add it. It was already there for decades.

@ChristianTackeGSI
Copy link
Member

we added the error message

If you mean recently, no, we did not add it. It was already there for decades.

Ohhh, okay.

Then: With the move to unique_ptr it became even more important?

@dennisklein
Copy link
Member

No, why?

@ChristianTackeGSI
Copy link
Member

Before 7f0f7f1 the passed volume was not destructed (and leaked). So any user of addVolume could still continue using the FairVolume.

With the destruction the error message became a little more important.

IMHO, if we remove the error message, we probably should remove the (deprecated) void addVolume(FairVolume* elem) as well. It makes it too easy to continue using an already destroyed object.

@dennisklein
Copy link
Member

So any user of addVolume could still continue using the FairVolume.

No, they could not. Even before the semantics was to transfer ownership. The caller had no chance to know otherwise.

@dennisklein
Copy link
Member

Or let me ask like this: give me a usage description for the void addVolume(FairVolume* elem) which is not broken.

karabowi added a commit to karabowi/FairRoot that referenced this issue Nov 27, 2024
@karabowi
fix(sim): Change log level for multiple sensitive volumes with same name
43fc099 
Both the transport engines and FairRoot allow registering
of multiple nodes with the same volume/same volume name (copy mechanism).
However currently such workflow logs errors from `FairVolumeList::addVolume()`.

The commit changes the LOG(error) to LOG(debug).

Fixes the issue FairRootGroup#1595.
karabowi added a commit to karabowi/FairRoot that referenced this issue Dec 18, 2024
@karabowi
fix(sim): Change and move log level for multiple sensitive volumes wi…
ee84b67 
…th same name

Both the transport engines and FairRoot allow registering
of multiple nodes with the same volume/same volume name (copy mechanism).
However currently such workflow logs errors from `FairVolumeList::addVolume()`,
which in turn is called by `FairModule::AddSensitiveVolume()`.

Since the `FairVolumeList::addVolume()` returns nullpointer
in such cases, the commit moves `LOG` and changes it severity
from `LOG(error)` in `FairVolumeList::addVolume()`
to `LOG(debug)` in `FairModule::AddSensitiveVolume()`.

Fixes the issue FairRootGroup#1595.
karabowi added a commit to karabowi/FairRoot that referenced this issue Dec 18, 2024
@karabowi
fix(sim): Clarify behaviour for same-name sensitive volumes
3fcd915 
The transport engines allow registering of multiple nodes
with the same volume/same volume name (copy mechanism).
In `FairRoot` this mechanism works provided unique volume name
over the whole geometry setup of different detectors.

The commit clarifies the situtation:
1. for same volume names in one detector, it quenches the log message
by moving `LOG` and changing it severity
from `LOG(error)` in `FairVolumeList::addVolume()`
to `LOG(debug)` in `FairModule::AddSensitiveVolume()`.
2. for same volume names accross different detectors,
it crashes the program with appropriate `LOG(fatal)`.

Fixes the issue FairRootGroup#1595.
karabowi added a commit to karabowi/FairRoot that referenced this issue Jan 22, 2025
@karabowi
fix(sim): Clarify behaviour for same-name sensitive volumes
718e712 
The transport engines allow registering of multiple nodes
with the same volume/same volume name (copy mechanism).
In `FairRoot` this mechanism works provided unique volume name
over the whole geometry setup of different detectors.

The commit clarifies the situtation:
1. for same volume names in one detector, it quenches the log message
by moving `LOG` and changing it severity
from `LOG(error)` in `FairVolumeList::addVolume()`
to `LOG(debug)` in `FairModule::AddSensitiveVolume()`.
2. for same volume names accross different detectors,
the program prints appropriate `LOG(fatal)`.

Fixes the issue FairRootGroup#1595.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@karabowi karabowi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@dennisklein @karabowi @TobiasStockmanns @ChristianTackeGSI