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

Remove bar size hardcoding #147

Open
jdkio opened this issue Sep 5, 2024 · 8 comments
Open

Remove bar size hardcoding #147

jdkio opened this issue Sep 5, 2024 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@jdkio
Copy link
Contributor

jdkio commented Sep 5, 2024

If we run with any other size bar (like when we adjust the geometry), we get

width of scinBoxlvTMS_PV_13 not as expected!
xwidth: 30.2
ywidth: 3096
zwidth: 10
terminate called without an active exception

That comes from this hardcoded check here:

if (zw != 10 || (xw != 35.42 && yw != 35.42)) {

That needs to be removed

@jdkio jdkio added the bug Something isn't working label Sep 5, 2024
@LiamOS
Copy link
Member

LiamOS commented Sep 9, 2024

Am adding myself and @AsaNehm on this, the Kalman filter will need a small modification to work with more general bar sizes. I imagine there are some numbers/indices/values in the reco code that assume bar geometry and/or angle that should be generalised.

Might be needed to allow TMS_Bar to present some necessary info to the code that needs it.

@AsaNehm
Copy link
Contributor

AsaNehm commented Sep 10, 2024

Files that have a hard-coded 35.42 in are (only in src)

  • TMS_Bar.cpp
  • TMS_Kalman.cpp
    So in principle this should be easy to change once we know where the value should be taken from. The bigger problem might be to change the call to where ever the width in xw or yw is referring to

@AsaNehm
Copy link
Contributor

AsaNehm commented Sep 11, 2024

The part in TMS_Bar.cpp is in addition inside a sanity check. So this could be taken out if we know that it works as it is supposed to or change the check from a throw to a message like width is not as in official design. Proceed at your own risk

@jdkio
Copy link
Contributor Author

jdkio commented Sep 13, 2024

Yeah, TMS is far enough along that we don't need guardrails. We can let users use whatever values they want without any checks (we don't want constant warning messages in the logs)

We need to improve tms_geom so that it can return the right answer for any geometry questions. As a first step, we should at least replace all hard coded numbers to TMS geometry function calls. Then we can adjust the functions as needed. We need to think about how to best get the bar width out of the geometry manager

@AsaNehm
Copy link
Contributor

AsaNehm commented Sep 23, 2024

One additional thing is that the thickness of the scintillator bars is also in many places hardcoded to 10mm. With the new design having 17mm thickness this is going to be a problem as well.
(edit: checked for this and it doesn't seem to be a problem in the reco so far)

@AsaNehm
Copy link
Contributor

AsaNehm commented Sep 23, 2024

Took out the sanity check in TMS_Bar.cpp

@jdkio
Copy link
Contributor Author

jdkio commented Nov 6, 2024

@AsaNehm, looks like the sanity check in TMS_Bar is still there. Did you forget to make a PR?

@AsaNehm
Copy link
Contributor

AsaNehm commented Nov 7, 2024

Didn't forget it but wanted to leave it open for the Kalman filter stuff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

3 participants