Updated default and backup STT geometry for SAND added. #34

nibir98 · 2024-11-18T15:40:01Z

The updated, default and backup geometries are added as a standalone build.

ast0815 · 2024-12-03T15:13:11Z

build_hall.sh

+           duneggd/Config/SAND_MAGNET.cfg \
+           duneggd/Config/SAND_INNERVOLOPT_DefaultSTT.cfg \
+           duneggd/Config/SAND_ECAL.cfg \
+           duneggd/Config/SAND_STT/STT_Default.cfg \


You have this "default geometry", but it is not actually used in the default that is built when no argument is given to "build_hall.sh". If this is the default, then it should be used for the "prod" options. Currently, that is still using "STT1.cfg", whatever that is.

The default configuration, has some volume naming changes with some slight modifications. The sandreco package is sensitive to volume names. In order to not break the sandreco code due to volume name conflicts, this is not added as part of the prod geometries as we talked about sometime ago. Moreover after discussion we have decided to rename the argument in the build_hall.sh script as default -> complete, backup -> initial respectively.

ast0815 · 2024-12-03T15:14:59Z

duneggd/Config/SAND_STT/STT_Backup.cfg

+# target
+FrameThickness          = Q("10cm")
+AddGapForSlab           = Q("7cm")
+#targetThickness         = {"CMod" : Q("4mm"),    "C3H6Mod": Q("5mm"), "C3H6ModNoRad": Q("6.89mm"), "TrkMod":Q("0mm"), "TrkMod2lyr":Q("0mm")}


What is with these commented out configurations? We are using version control, so unless there is a very good reason, unused code should just be removed and not be left in as comments. Especially if there are no explanation in the comments why it is commented out.

Same in multiple places in the code.

The commented section is cleared now.

ast0815 · 2024-12-03T15:15:59Z

duneggd/SubDetector/SandInnerVolume.py

@@ -28,9 +28,13 @@ def build_tracker(self, main_lv, geom):
        # if "STT" not in self.builders:
        #     print("STT builder not found")
        #     return  
+        #print(self.builders)


See above comment about commented out code.

this has been cleared.

ast0815

Ok, this looks fine to me. I just suggested a change to the changelog entry. Please check if this is acceptable.

Just be aware, that since your geometry changes are not part of the "production" detector hall geometry, they have not been tested by the CI for overlaps. You have to do that yourself with the included scripts.

When you have modified the changelog entry and are certain the geometry is correct, I am happy to merge this.

ast0815 · 2024-12-04T10:09:10Z

CHANGELOG.md

@@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to a modified version of [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 Geometry releases will be tagged as `Descriptive_tag_v_X.Y.Z`.

+### Added
+
+- Updated STT geometry with Default and Backup options. Can be built as a standalone geometry. `build_hall.sh only_sand_def_stt` `build_hall.sh only_sand_bck_stt`


Suggested change

- Updated STT geometry with Default and Backup options. Can be built as a standalone geometry. `build_hall.sh only_sand_def_stt` `build_hall.sh only_sand_bck_stt`

- Added STT geometry with Complete and Initial options. Can be built as a standalone geometry: `build_hall.sh only_sand_def_stt` `build_hall.sh only_sand_initial_stt`. Not yet part of the default "production" detector hall.

Updated the log file.

Updated to the makefile method from the build_hall.sh method

It looks like those changes are not actually reflected in the branch? Also, please put this "added" line under the "unreleased" section rather than putting it at the top of the file.

ast0815 · 2024-12-05T09:17:47Z

Also, does this address any of the SAND-related issues in the issue tracker?

ast0815 · 2024-12-05T15:41:44Z

I have changed the way we build the default geometries. They are now defined in a Makefile rather than a bash script. Could you pull those changes from master and add your new geometries as a target there?

ast0815 · 2024-12-09T09:21:22Z

CHANGELOG.md

 ## [Unreleased]

+### Added
+- Updated with standalone build of initial and complete STT configurations. `only_SAND_STT_Initial.gdml` `only_SAND_STT_Complete.gdml`


There is already an 'Added' subsection below. Please just add your line there.

ast0815 · 2024-12-09T09:22:53Z

Makefile

+		   duneggd/Config/SAND_STT/STT_Default.cfg \
+		   duneggd/Config/SAND_GRAIN.cfg \
+
+


Could you please follow the indentation convention in this file and use a single tab character?

ast0815 · 2024-12-09T16:10:59Z

CHANGELOG.md

+### Added
+


These two lines should not be added.

ast0815 · 2024-12-10T09:49:28Z

@nibir98 So, did this address any of the open issues in the repository related to SAND geometry?

ast0815 · 2024-12-10T11:05:29Z

Also, can I delete this branch now?

nibir98 · 2024-12-10T15:17:35Z

Hi Lukas, thanks for your help. The branch can be deleted.

Updated default and backup STT geometry for SAND added.

fcc6c7c

nibir98 requested review from ast0815 and GianfrancoIngratta November 18, 2024 15:40

ast0815 requested changes Dec 3, 2024

View reviewed changes

Address prod related questions. Removed commented lines.

779f39c

ast0815 requested changes Dec 4, 2024

View reviewed changes

Nibir Talukdar added 2 commits December 5, 2024 19:08

Merge branch 'master' into stt_opt_def_bck

b73b3fe

Adapted to the Makefile format

53c3224

ast0815 requested changes Dec 9, 2024

View reviewed changes

Nibir Talukdar added 2 commits December 9, 2024 09:39

Minor changes to indentation style in Makefile and Changelog.md

11aaf39

Some more indentation fix in Makefile

6700024

ast0815 reviewed Dec 9, 2024

View reviewed changes

CHANGELOG.md Outdated

Comment on lines 9 to 10

### Added

Copy link

Member

ast0815 Dec 9, 2024

Choose a reason for hiding this comment

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

These two lines should not be added.

Changes to Changelog.md

8f4e925

ast0815 approved these changes Dec 10, 2024

View reviewed changes

ast0815 merged commit 7ed6d8a into master Dec 10, 2024
3 checks passed

ast0815 mentioned this pull request Dec 10, 2024

STT default geometry added #26

Closed

ast0815 deleted the stt_opt_def_bck branch December 10, 2024 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated default and backup STT geometry for SAND added. #34

Updated default and backup STT geometry for SAND added. #34

nibir98 commented Nov 18, 2024

ast0815 Dec 3, 2024

nibir98 Dec 3, 2024

ast0815 Dec 3, 2024

nibir98 Dec 3, 2024

ast0815 Dec 3, 2024

nibir98 Dec 6, 2024

ast0815 left a comment

ast0815 Dec 4, 2024

nibir98 Dec 6, 2024

nibir98 Dec 6, 2024

ast0815 Dec 6, 2024

ast0815 commented Dec 5, 2024

ast0815 commented Dec 5, 2024

ast0815 Dec 9, 2024

ast0815 Dec 9, 2024

ast0815 Dec 9, 2024

ast0815 commented Dec 10, 2024

ast0815 commented Dec 10, 2024

nibir98 commented Dec 10, 2024

	- Updated STT geometry with Default and Backup options. Can be built as a standalone geometry. `build_hall.sh only_sand_def_stt` `build_hall.sh only_sand_bck_stt`
	- Added STT geometry with Complete and Initial options. Can be built as a standalone geometry: `build_hall.sh only_sand_def_stt` `build_hall.sh only_sand_initial_stt`. Not yet part of the default "production" detector hall.

		duneggd/Config/SAND_STT/STT_Default.cfg \
		duneggd/Config/SAND_GRAIN.cfg \

Updated default and backup STT geometry for SAND added. #34

Updated default and backup STT geometry for SAND added. #34

Conversation

nibir98 commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ast0815 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ast0815 commented Dec 5, 2024

ast0815 commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ast0815 commented Dec 10, 2024

ast0815 commented Dec 10, 2024

nibir98 commented Dec 10, 2024