Readout plane upgrade, readout validation and new library version #92

jgalan · 2023-06-07T10:01:47Z

Library version increased to v2.0

The aim of this PR is to add the new metadata members required for future implementation of readout plane rotation and orientation so that the new readouts can be already pre-generated for pipeline validation.

We have introduced new members inside TRestDetectorReadoutPlane.

fAxisX and fAxisY. They are initialised internally using the fNormal value.
fTotalDriftDistance disappears, and it is replaced by fHeight which is defined by user.
fCathode position will now disappear. We just define the height of the active volume above the readout plane. There is still a method GetCathodePosition that returns the readout position at a given fHeight.

Related to: rest-for-physics/framework#438

…tor to fNormal

…method

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

lobis · 2023-06-07T11:13:31Z

I am confused, should we not use #85 for these changes? should I just close it?

…l vector

…ics/detectorlib into jgalan-readoutplane-update

jgalan · 2023-06-07T11:27:11Z

I am confused, should we not use #85 for these changes? should I just close it?

No, just merge it, I wanted to create a reduced one with the final metadata members (without final implementation) we will need for the readout. Because readouts need to be regenerated with the new metadata members for the pipelines, and we need to fix a new release.

lobis · 2023-06-07T11:31:55Z

I am confused, should we not use #85 for these changes? should I just close it?

No, just merge it, I wanted to create a reduced one with the final metadata members (without final implementation) we will need for the readout. Because readouts need to be regenerated with the new metadata members for the pipelines, and we need to fix a new release.

Then I have one more comment before this is frozen: I think it would make more sense to store a height parameter instead of the cathode position. The readout position (TVector), normal vector and height (along the normal vector) should be enough to completely define the readout in space. We could still support the users to define the readout using a point in the cathode plane but a single parameter height should also be supported (in my opinion) and I think it makes more sense to store it as a single parameter. Also I am not sure I like the idea of naming the opposite plane to the readout as "cathode" as not all readouts use drift for reading (light?) we should perhaps use more generic words.

jgalan · 2023-06-08T08:52:00Z

I am confused, should we not use #85 for these changes? should I just close it?

No, just merge it, I wanted to create a reduced one with the final metadata members (without final implementation) we will need for the readout. Because readouts need to be regenerated with the new metadata members for the pipelines, and we need to fix a new release.

Then I have one more comment before this is frozen: I think it would make more sense to store a height parameter instead of the cathode position. The readout position (TVector), normal vector and height (along the normal vector) should be enough to completely define the readout in space. We could still support the users to define the readout using a point in the cathode plane but a single parameter height should also be supported (in my opinion) and I think it makes more sense to store it as a single parameter. Also I am not sure I like the idea of naming the opposite plane to the readout as "cathode" as not all readouts use drift for reading (light?) we should perhaps use more generic words.

I agree, the cathode can just disappear.

We can just call it fDriftDistance? But that would be not generic. Perhaps fActiveVolumeHeight?

lobis · 2023-06-08T08:59:25Z

I am confused, should we not use #85 for these changes? should I just close it?

No, just merge it, I wanted to create a reduced one with the final metadata members (without final implementation) we will need for the readout. Because readouts need to be regenerated with the new metadata members for the pipelines, and we need to fix a new release.

Then I have one more comment before this is frozen: I think it would make more sense to store a height parameter instead of the cathode position. The readout position (TVector), normal vector and height (along the normal vector) should be enough to completely define the readout in space. We could still support the users to define the readout using a point in the cathode plane but a single parameter height should also be supported (in my opinion) and I think it makes more sense to store it as a single parameter. Also I am not sure I like the idea of naming the opposite plane to the readout as "cathode" as not all readouts use drift for reading (light?) we should perhaps use more generic words.

I agree, the cathode can just disappear.

We can just call it fDriftDistance? But that would be not generic. Perhaps fActiveVolumeHeight?

I think perhaps fHeight should be clear enough.

…ics/detectorlib into jgalan-readoutplane-update

jgalan · 2023-06-08T16:02:45Z

From this point I expect we are not going to modify any more the data members, so I use the version at this point to regenerate validation readouts. TREX-DM, etc.

…rface to access module size

…on needs to be worked out in degrees

…ditional check

…llection

jgalan · 2023-06-14T07:25:59Z

src/TRestDetectorReadoutPlane.cxx

@@ -504,7 +504,8 @@ void TRestDetectorReadoutPlane::UpdateAxes() {  // idempotent
 }

 void TRestDetectorReadoutPlane::SetRotation(Double_t radians) {
-    fRotation = radians;
+    // sets fRotation modulo 2pi
+    fRotation = TVector2::Phi_0_2pi(radians);


In principle, why radians would be above 2PI? It could be a way to identify a problem. For example if I do something like plane->SetRotation(180) I will see soon that when transforming to degrees somewhere else in the code, the value is out of range.

We could add a warning then whenever it goes over 2pi. I think it makes more sense to store rotation between 0 and 2pi in general (not just in this class).

jgalan · 2023-06-14T07:29:24Z

src/TRestDetectorReadoutPlane.cxx

@@ -523,3 +523,17 @@ TVector3 TRestDetectorReadoutPlane::GetPositionInWorld(const TVector2& point, Do
 Double_t TRestDetectorReadoutPlane::GetDistanceToPlane(const TVector3& point) const {
    return (point - fPosition).Dot(fNormal);
 }
+
+void TRestDetectorReadoutPlane::SetAxisX(const TVector3& axis) {


When or why we want to do this?

It could be useful for testing for example.

…ics/detectorlib into jgalan-readoutplane-update

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

jgalan · 2023-06-14T10:24:15Z

Ok pipeline is now green. It means that now I have to get back to master on the framework validation?

I understand the pipeline will fail, but once the PR is merged we are back to normal.

Also rest-for-physics/framework#438 is green

…ics/detectorlib into jgalan-readoutplane-update

jgalan added 3 commits June 7, 2023 11:45

TRestDetectorReadoutPlane. Added fAxisX and fAxisY. Renamed fPlaneVec…

8484f5b

…tor to fNormal

Increasing library version to 2.0

ecb8590

Adding default values

5737989

jgalan changed the title ~~Readoutplane preparations~~ Readoutplane preparations and new library version Jun 7, 2023

jgalan changed the title ~~Readoutplane preparations and new library version~~ Readout plane upgrade, readout validation and new library version Jun 7, 2023

jgalan and others added 7 commits June 7, 2023 12:33

TRestDetectorReadoutPlane. Adding Setters/Getters

cba956e

TRestDetectorReadout. Reading out plane axis Y and Y

949bb89

TRestDetectorReadoutPlane. Updating Print information

14b5923

Updating processes to renamed TRestDetectorReadoutPlane::GetNormal() …

f0af64b

…method

TRestDetectorReadoutPlane::Print. Fixing typo

6ec8fcc

Updating readout

ca7f8a2

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

87f4cb3

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

jgalan added 2 commits June 7, 2023 13:24

TRestDetectorReadout. X and Y plane axis are determined only by norma…

da5c462

…l vector

Merge branch 'jgalan-readoutplane-update' of github.com:rest-for-phys…

6c633c6

…ics/detectorlib into jgalan-readoutplane-update

lobis and others added 10 commits June 8, 2023 11:02

fix typos

6b01b08

remove fNModules field

507c96d

fix typos

7fe541f

Using single parameter height instead of TVector3 cathode position

4f06a6e

simplified initialization

af6b85f

remove #pragma region (local IDE settings should not be committed)

0368727

TRestDetectorReadout. Changing the sign of x-axis

2af0c73

Readout updates

7a05e4e

Merge branch 'jgalan-readoutplane-update' of github.com:rest-for-phys…

d003735

…ics/detectorlib into jgalan-readoutplane-update

Fixing compilation issues

d1482dd

lobis and others added 18 commits June 9, 2023 12:59

remove confusing GetSizeZ method (should be called Y?). Single inte…

844841b

…rface to access module size

TRestDetectorReadoutPlane. Increasing class version

da3356b

TRestDetectorReadoutPlane. Recovering fAxisX and fAxisY

9d3b51d

remove #pragma region directive

e0f4578

TRestDetectorReadoutPlane. Fixing a piece of code where module rotati…

d1521c2

…on needs to be worked out in degrees

prevent user from setting zero vector as normal

e78c227

verify axes and normal are orthonormal reference frame

8e40b13

handle case when normal is exactly opposite to originalNormal. Add ad…

cc014aa

…ditional check

update axes initialization. Add tests for axes

2cd0ca7

TRestDetectorReadout. Adding default values for rotation and chargeCo…

65a2f67

…llection

generateReadout.rml removing units from normal definition

95b166f

TRestDetectorReadout. Charge collection default value removed

a9831e7

readout/validation.txt Fixing validation axis orientation

9a67237

pipeline/generateReadout.rml adding units

05dafc9

methods to go from/to global/local coords

5a21b30

rotation is now saved as a value from 0 to 2pi

0fe6b35

add method to set plane x-axis explicitly. Add validation

b244133

rename originalNormal to zUnit (internal method)

a5159dc

jgalan commented Jun 14, 2023

View reviewed changes

jgalan and others added 3 commits June 14, 2023 10:46

Transferring basic-readouts validation

92da00a

Merge branch 'jgalan-readoutplane-update' of github.com:rest-for-phys…

df24542

…ics/detectorlib into jgalan-readoutplane-update

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

ee94ffb

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

jgalan mentioned this pull request Jun 14, 2023

Updating pipelines for readout updates rest-for-physics/framework#438

Merged

lobis approved these changes Jun 14, 2023

View reviewed changes

jgalan added 2 commits June 14, 2023 12:26

Updating to master validation

4c9606b

Merge branch 'jgalan-readoutplane-update' of github.com:rest-for-phys…

c912c30

…ics/detectorlib into jgalan-readoutplane-update

jgalan merged commit 11822a7 into master Jun 14, 2023

jgalan deleted the jgalan-readoutplane-update branch June 14, 2023 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Readout plane upgrade, readout validation and new library version #92

Readout plane upgrade, readout validation and new library version #92

jgalan commented Jun 7, 2023 •

edited by pull-request-badge bot

Loading

lobis commented Jun 7, 2023

jgalan commented Jun 7, 2023 •

edited

Loading

lobis commented Jun 7, 2023 •

edited

Loading

jgalan commented Jun 8, 2023

lobis commented Jun 8, 2023 •

edited

Loading

jgalan commented Jun 8, 2023 •

edited

Loading

jgalan Jun 14, 2023

lobis Jun 14, 2023

jgalan Jun 14, 2023

lobis Jun 14, 2023

jgalan commented Jun 14, 2023

Readout plane upgrade, readout validation and new library version #92

Readout plane upgrade, readout validation and new library version #92

Conversation

jgalan commented Jun 7, 2023 • edited by pull-request-badge bot Loading

lobis commented Jun 7, 2023

jgalan commented Jun 7, 2023 • edited Loading

lobis commented Jun 7, 2023 • edited Loading

jgalan commented Jun 8, 2023

lobis commented Jun 8, 2023 • edited Loading

jgalan commented Jun 8, 2023 • edited Loading

jgalan Jun 14, 2023

Choose a reason for hiding this comment

lobis Jun 14, 2023

Choose a reason for hiding this comment

jgalan Jun 14, 2023

Choose a reason for hiding this comment

lobis Jun 14, 2023

Choose a reason for hiding this comment

jgalan commented Jun 14, 2023

jgalan commented Jun 7, 2023 •

edited by pull-request-badge bot

Loading

jgalan commented Jun 7, 2023 •

edited

Loading

lobis commented Jun 7, 2023 •

edited

Loading

lobis commented Jun 8, 2023 •

edited

Loading

jgalan commented Jun 8, 2023 •

edited

Loading