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

[tmadlener]

I think you can make this a comment block using <!-- and -->. That way the text would still show up for people if they open a PR, but it would not clutter the PR once it has been submitted, because it won't be visible. This obviously depends a bit on whether you want to make it harder to ignore or not ;)

[tmadlener]

I am also not sure if PRs also have the fancy features that issues have (see, e.g. DD4hep issues)

[BrieucF]

Yes, I think we can make it a comment block (it is anyway easy to ignore :-P )

[andresailer]

People are going to ignore it anyway, so if we make it checkboxes, we can see from the PR overview that we can also ignore the PR!

[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.

[andresailer]

Why the XML comment signs?

Change everything into checkboxes?

[andresailer]

Suggested change

	To ease the review process, please consider the following before to open the pull request:
	- is your code well commented?
	- did you update the relevant README(s)? See e.g. https://github.com/key4hep/k4geo/blob/main/detector/calorimeter/README.md or https://github.com/key4hep/k4geo/blob/main/FCCee/IDEA/compact/README.md
	- if you added some code, is it covered by tests? See https://github.com/key4hep/k4geo/blob/main/lcgeoTests/CMakeLists.txt
	- is your PR branch up to date with origin 'main'?
	- are you not adding/modifying files that should not have been added/modified (or at least, not in this PR)?
	- if you added/modified a detector geometry, have you checked that was no overlap? You can do so with the following command: ddsim --compactFile PATH_TO_COMPACT_FILE --runType run --ui.commandsInitialize "/geometry/test/run" >> overlapDump.txt
	To ease the review process, please consider the following before to open the pull request:
	- [ ] the code is sufficiently well documented
	- [ ] the relevant README(s) were updated. See e.g. https://github.com/key4hep/k4geo/blob/main/detector/calorimeter/README.md or https://github.com/key4hep/k4geo/blob/main/FCCee/IDEA/compact/README.md
	- [ ] the code is covered by tests. See https://github.com/key4hep/k4geo/blob/main/test/CMakeLists.txt
	- [ ] the PR source branch has been rebased (`--ff-only`) to `k4geo/main`
	- [ ] the PR does not contain any additions or modifications that do not belong to it
	- [ ] the changes in this PR have not introduced any overlaps. You can do so with the following command: ddsim --compactFile PATH_TO_COMPACT_FILE --runType run --ui.commandsInitialize "/geometry/test/run" >> overlapDump.txt

[andresailer]

Suggested change

	exactly as it appears between the two bold lines
	exactly as it appears between the two ALL CAPS lines

[andresailer]

Suggested change

	If your modifications are compliant with the above, populate the release note text below, and open the PR!
	- [ ] The release notes below contain a succinct and comprehensive description of the changes that are sufficiently self-explanatory

[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.