Include common review comments in the PR template

[BrieucF]

I open this PR to discuss the content of .github/PULL_REQUEST_TEMPLATE

BEGINRELEASENOTES

• Include common review comments in the PR template

ENDRELEASENOTES

[jmcarcell]

Another question would be if we want the same templates for all of the Key4hep repositories or not; at least the questions in the template in this PR are quite generic and don't need to refer to the exact file in the current repository.

[BrieucF]

Another question would be if we want the same templates for all of the Key4hep repositories or not; at least the questions in the template in this PR are quite generic and don't need to refer to the exact file in the current repository.

That is a good point, what is written there is indeed very generic. The only thing is that I wanted to link the README(s) and the tests here because people might not know where they are... But ok, this is not a super strong argument, I let you guys decide

[tmadlener]

I think (most of) the other Key4hep ones already pull theirs from the central templates. k4geo is likely a special case due to its history and the fact that it was originally in the iLCSoft organisation.

I have no strong opinion either, but as Brieuc has already pointed out two things that are somewhat k4geo specific, I would lean slightly towards keeping the dedicated template here.

[atolosadelgado]

There is a battery of tests I do for geometries, can we add these as check boxes, at least it would be useful for ourselves as Andre suggested :)

• overlap check (maybe increasing the number of points up to 100k or more). I think this one is already in the list
• simulation with geantinos, 300k or more. If error is print out during navigation, one has to go back to previous point. Verbosity can be increased to catch inconsistencies
• material scan. This one is more handmade, because one has to spot the error by oneself (that is, if you expect aluminum and the geantinos do not see aluminum, there is a problem)
• simulation with physical particles (particle gun), which can trigger errors during navigation (as it was the case of the twisted tube)

Even with default verbosity, the amount of text printed out is too much, so I usually redirect the output to a file and then look for some keywords such as error and G4-Exception.

The tests of the simulation with physical particles and geantinos are also useful to get an idea of the speed.

[BrieucF]

Sorry for the long silence, here is a new version.

[atolosadelgado]

Hi,

The transition from k4simgeant4 to DDSim has revealed that many subsystems lack defined regions and limits, which may compromise accuracy. I recommend attaching a region to each subsystem, specifying the production cut and optionally associating it with appropriate limits.

Some information about the Region [link, link] and the associated user limits [link] can be found in Geant4 documentation.

[tmadlener]

Should we consider splitting this list into more than one? E.g. one that definitely almost always applies (e.g. test coverage, README updates, etc..) and one (or more) other ones that apply for entirely new developments or significant changes.

How long does an overlap check take? Can we run that in CI automatically? Everything that doesn't have to be done manually is less work for everyone involved.

[atolosadelgado]

Should we consider splitting this list into more than one? E.g. one that definitely almost always applies (e.g. test coverage, README updates, etc..) and one (or more) other ones that apply for entirely new developments or significant changes.

How long does an overlap check take? Can we run that in CI automatically? Everything that doesn't have to be done manually is less work for everyone involved.

The overlap check can take several minutes already for 1 subsystem, but some tricks may be applied to speed it up. I added dedicated tests for ARC and the drift chamber [link], but they are heavily customized and standalone.

Recently, I noticed that simulation of Geantinos is also useful to spot problems, and it may be better suited for big detectors (10M geantinos should be fast and they would cover the 4pi with some granularity). Adding this check to the CI tests looks like a good idea to me, thanks for the question!

[BrieucF]

Here a new version with comments implemented. I did not add a requirement on the detector region/limits, I feel like it is a bit too detailed, but feel free to propose a sentence if you think it is important and should be included here.

I propose to tackle the 'overlap check in CI' in another PR.