Cigar #258
Cigar #258
Conversation
|
Hi @MarcSeemann! I've noticed that your branch is quite old (in fact, it doesn't compile with the last Geant4 version). I suggest you rebase it on master before we can go through a review. |
|
Hey Dr. Paola Ferrario,Sorry I didn‘t mean to try and push to main. But yes, if I ever want to I will make sure it works with the latest version.Kind Regards,MarcAm 24.06.2024 um 11:37 schrieb Paola Ferrario ***@***.***>:
Hi @MarcSeemann! I've noticed that your branch is quite old (in fact, it doesn't compile with the last Geant4 version). I suggest you rebase it on master before we can go through a review.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
It's just that for anyone to review your code properly, it needs to compile and it doesn't at the moment. I can hack it locally and make it compile for now, but you should rebase it, and the sooner the better. |
|
There are a lot of geometry overlaps in the code. Review each one of them, because they could bring to an unexpected behaviour in tracking. |
| @@ -0,0 +1,26 @@ | |||
| ### -------------------------------------------------------- | |||
| ### This init macro simulates the response of a SensL SiPM DICE | |||
There was a problem hiding this comment.
Update the description of the macro to what it actually does.
| @@ -0,0 +1,21 @@ | |||
| #/Generator/SingleParticle/particle opticalphoton | |||
There was a problem hiding this comment.
Add the description of the macro (same text as the init file).
|
|
||
| /nexus/RegisterRunAction DefaultRunAction | ||
| /nexus/RegisterTrackingAction DefaultTrackingAction | ||
| /nexus/RegisterTrackingAction OpticalTrackingAction |
There was a problem hiding this comment.
The OpticalTrackingAction saves all optical photons. Why do you need that? It creates very huge files and should be used only for debugging. Please, use the default tracking action instead.
| @@ -0,0 +1,64 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Remove this file, you may have added it by mistake.
| @@ -0,0 +1,21 @@ | |||
| #/Generator/SingleParticle/particle opticalphoton | |||
|
|
||
| new G4LogicalSkinSurface("TEFLON_OPSURF", teflon_logic_top, opsur_teflon); | ||
|
|
||
| G4VPhysicalVolume *teflon_top = new G4PVPlacement(0, G4ThreeVector(0, cigar_width_/2+panel_width/2+fiber_diameter_+extra_width/2+panel_width/2+fiber_diameter_/4,0), |
There was a problem hiding this comment.
You don't need to define a variable for the placement, if you're not going to use it later. You can just write:
new G4PVPlacement(...)
| // G4LogicalVolume* teflon_logic_front = | ||
| // new G4LogicalVolume(teflon_closing_panel, teflon, "TEFLON"); | ||
| // teflon_logic_front->SetVisAttributes(nexus::White()); | ||
| G4VSolid* temp_solid = teflon_closing_panel; |
There was a problem hiding this comment.
Why are you redefining this variable?
| // new G4LogicalVolume(teflon_closing_panel, teflon, "TEFLON"); | ||
| // teflon_logic_front->SetVisAttributes(nexus::White()); | ||
| G4VSolid* temp_solid = teflon_closing_panel; | ||
| G4double fiber_radius = fiber_diameter_/1.1; |
There was a problem hiding this comment.
The name of this variable is somewhat misleading: a radius should be a diameter/2. In line 328 it becomes evident that it is not a radius, so change the name to a more meaningful one.
|
|
||
| std::string label = std::to_string(ifiber); | ||
|
|
||
| G4PVPlacement* fiber_placement1 = new G4PVPlacement(0, G4ThreeVector(cigar_width_ / 2 + fiber_diameter_ / 2+extra_width/2+panel_width/2+fiber_diameter_/4, -cigar_width_ / 2 + ifiber * fiber_diameter_ + fiber_diameter_ / 2, 3.5 * cm), |
There was a problem hiding this comment.
Review the length of the lines following the nexus coding conventions.
| ### board in the black box. | ||
| ### -------------------------------------------------------- | ||
|
|
||
| /control/execute macros/physics/DefaultPhysicsList.mac |
There was a problem hiding this comment.
If you use the default physics, do not repeat the 13-18 lines. Make sure you actually want to use the default list, otherwise, just remove this line.
No description provided.