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

IT3: Support lightweight GeometryTGeo #12769

Merged
merged 3 commits into from
Mar 5, 2024
Merged

Conversation

f3sch
Copy link
Collaborator

@f3sch f3sch commented Feb 26, 2024

Changes included here:

  • Setting detector name from extracted geometry.
  • fixed a bug that the IT3 delta for pitch sizes would also be applied to ITS when built with ENABLE_UPGRADES (which is of course nonsense, since there are no use cases for building with this flag and then simulating ITS not IT3, but fixed to be self-consistent).
  • When creating the aligned geometry and lightweight GeometryTGeo only do the latter for the detectors actually present in the mask.

Copy link
Contributor

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass
async-2023-pp-apass1
async-2022-pp-apass6
async-2022-pp-apass4
async-mc
async-data

@shahor02
Copy link
Collaborator

Note that I asked @mconcas to close the da64d8a since using 3D field in the ITS did not show any advantage in the precision but caused ~4% slowdown.

@f3sch
Copy link
Collaborator Author

f3sch commented Feb 26, 2024

Sorry @shahor02, I do not understand your point. In #12761 da64d8a is added, no?
I simply took the patch from the PR and applied it locally.
I do not plan to merge the three preceding commits that are attached but will rebase to whatever is dev after #12765 is merged.

@shahor02
Copy link
Collaborator

Ah, actually, it sneaked into my PR because I was testing 2 things at the same time, will remove it before merging.

@f3sch
Copy link
Collaborator Author

f3sch commented Feb 26, 2024

Ok understood, thanks.

@f3sch f3sch force-pushed the it3/nogeom branch 3 times, most recently from 3947475 to bc20007 Compare February 27, 2024 10:44
@f3sch f3sch marked this pull request as ready for review February 27, 2024 10:52
mconcas
mconcas previously approved these changes Feb 27, 2024
Copy link
Collaborator

@mconcas mconcas left a comment

Choose a reason for hiding this comment

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

ITS part almost untouched, few minor comments.

Detectors/ITSMFT/ITS/base/src/GeometryTGeo.cxx Outdated Show resolved Hide resolved
Signed-off-by: Felix Schlepper <[email protected]>
Signed-off-by: Felix Schlepper <[email protected]>
@f3sch
Copy link
Collaborator Author

f3sch commented Feb 28, 2024

reverting to draft until a resolution for O2-4698 is found.

@f3sch f3sch marked this pull request as draft February 28, 2024 20:16
@shahor02
Copy link
Collaborator

Well, since you don't modify the data members, it should not be related to O2-4698.

@f3sch f3sch marked this pull request as ready for review February 29, 2024 11:17
@alibuild
Copy link
Collaborator

Error while checking build/O2/fullCI for e66e1bc at 2024-02-29 16:15:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'


## sw/BUILD/O2-full-system-test-latest/log
task timeout reached .. killing all processes


## sw/BUILD/o2checkcode-latest/log
--
========== List of errors found ==========
++ GRERR=0
++ grep -v clang-diagnostic-error error-log.txt
++ grep ' error:'
++ GRERR=1
++ [[ 1 == 0 ]]
++ mkdir -p /sw/INSTALLROOT/a8f1014e1a77f08e2848dfd993deba85b6fef987/slc8_x86-64/o2checkcode/1.0-local141/etc/modulefiles
++ cat
--

Full log here.

@f3sch
Copy link
Collaborator Author

f3sch commented Mar 5, 2024

@shahor02 can this be merged? thanks.

@shahor02 shahor02 merged commit 14189ae into AliceO2Group:dev Mar 5, 2024
11 of 12 checks passed
andreasmolander pushed a commit to andreasmolander/AliceO2 that referenced this pull request Apr 12, 2024
* IT3: Support lightweight GeometryTGeo

Signed-off-by: Felix Schlepper <[email protected]>

* IT3: fix spelling

Signed-off-by: Felix Schlepper <[email protected]>

* IT3: Update README

Signed-off-by: Felix Schlepper <[email protected]>

---------

Signed-off-by: Felix Schlepper <[email protected]>
andreasmolander pushed a commit to andreasmolander/AliceO2 that referenced this pull request Apr 12, 2024
* IT3: Support lightweight GeometryTGeo

Signed-off-by: Felix Schlepper <[email protected]>

* IT3: fix spelling

Signed-off-by: Felix Schlepper <[email protected]>

* IT3: Update README

Signed-off-by: Felix Schlepper <[email protected]>

---------

Signed-off-by: Felix Schlepper <[email protected]>
mwinn2 pushed a commit to mwinn2/AliceO2 that referenced this pull request Apr 25, 2024
* IT3: Support lightweight GeometryTGeo

Signed-off-by: Felix Schlepper <[email protected]>

* IT3: fix spelling

Signed-off-by: Felix Schlepper <[email protected]>

* IT3: Update README

Signed-off-by: Felix Schlepper <[email protected]>

---------

Signed-off-by: Felix Schlepper <[email protected]>
@f3sch f3sch deleted the it3/nogeom branch November 25, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants