Revert "Implement particle filtering for neuland simulation"

[jose-luis-rs]

Reverts #1083

[YanzhaoW]

@jose-luis-rs @hapol

I'm sorry that I don't understand this reversion. The original PR #1083 was open for more than two weeks and I didn't see any other people left any comments over there. It passed all the CIs without any issue and so I requested the merge.

I'm confused why this reversion was done without giving any reasons and without asking the opinion of the PR author and other people. If I'm not wrong, we have this one week limitation before any changes (except small bug fixes) are merged to the dev branch as well as the passing of our CI pipelines. But the PR of this reversion was merged almost immediately with the CI tests failed.

[jose-luis-rs]

Hello @YanzhaoW
This was a request from the NeuLAND WG. Please, try to discuss the modifications with your WG and then we will merge your PR again.

[igasparic]

Sorry for making confusion and that I didn't react before. But whenever files which are in use (e.g. R3BNeulandDigitizer.cxx in this case) are modified, I would like to know why. For instance why variable fDigitizingEngine is changed to digitizing_engine_. Ok, I learned meanwhile it is a trivial change in order to follow top standards in the IT business but it is confusing for me. Especially when the rest of the code keeps the convention fSomething.
Yes, I should have asked this questions here before but I also had asked that all these changes are presented in the WG meeting before they are submitted. Since the Neuland WG is under construction now I suggest this is presented in the Analysis WG meeting.

[YanzhaoW]

Hi, @igasparic

Sorry that I somehow missed the requirement that the changed code has to be presented in the WG meeting before it's being merged. Sure, I will present and explain the changes in the next WG meeting.

Since the Neuland WG is under construction now I suggest this is presented in the Analysis WG meeting.

I'm not sure whether it's suitable to discuss the code details in the Analysis WG meeting. I haven't seen anyone doing this before and I assume it's not a right place to present the technical details, which most of people are not involved with. But of course, if Valerri agrees that we could start to do the code review line by line, I could also do it in the analysis meeting.

Hi, @jose-luis-rs

The thing still confusing for me is that I think we had a standard decided in the analysis meeting, that any changes (except small changes for bug fixing) on the existing dev branch, should stay open for discussion for at least one week before being deployed to the dev branch. And the changes should pass all CI tests before the merging. But for this reversion, it failed most of CI test and got merged almost immediately. I'm very confused why the standard didn't apply here.

[jose-luis-rs]

Hi, @jose-luis-rs

The thing still confusing for me is that I think we had a standard decided in the analysis meeting, that any changes (except small changes for bug fixing) on the existing dev branch, should stay open for discussion for at least one week before being deployed to the dev branch. And the changes should pass all CI tests before the merging. But for this reversion, it failed most of CI test and got merged almost immediately. I'm very confused why the standard didn't apply here.

Hello @YanzhaoW

A reversion does not introduce new modifications in the code since it corresponds to a previous commit and we know that this already passed all CI tests. So in this specific case we don't need to wait one week.
The CI tests failed due to an external problem with the CERN packages and obviously this is not related to our code, today the full system is working as usual.

[morze00]

Hello @YanzhaoW

It is no problem to discuss and show the code during Analysis WG meetings, especially if this is in the agenda. In fact we do it sometimes. If you like you can present it next Thursday, 12th December. Maybe it is not necessary to discuss the entire code line by line, just the most important parts and modifications.

[YanzhaoW]

@jose-luis-rs

Ok, I have two questions regarding the reversion.

1. Under which conditions can a commit be reverted? For this reversion, it was done so quickly even before any conclusion is drawn in NeuLAND WG group. And the PR is merged instantly without allowing any opinions from original submitters. Will it be like this for the future commits from our contributors?
2. If reversion is outside this one week limit. Logically, is it correct to assume the reversion of the reversion also has no such one week limit?

@morze00

Sure, I'm glad to do this in the analysis meeting. Maybe before we start, we should clarify what kind of rules that we have for each PR. Like what kind PRs must be presented in analysis meeting before being merged. What should the content be? Should it be the results? and the code details? or something else? Should the PR be presented in WG group meeting or analysis meeting? And if the PR involves the changes of multiple directories, in which WG group should the changes be presented? For me, those answers are not very clear. So maybe we should clarify for our potential contributors and avoid any inconvenience in the future.

[hapol]

Hi @YanzhaoW,
Thank you for your comments. Regarding your questions and comments about the reversion: To my understanding, @jose-luis-rs was simply reverting the code to the version immediately prior to the merge. Therefore, no additional checks are necessary, as he has already explained.
In general, a commit might be reverted if there are indications that it is causing unexpected effects or errors in the code that could not be easily identified earlier, or if there's another situation that justifies reverting to a more stable version. However, this is not the case here. In this instance, the reversion was made in response to a WG request, so that the modifications could be discussed further within the WG or during an analysis meeting. This is not a new requirement, but rather a reasonable procedure for teamwork. Several previous major changes have followed a similar process, where modifications were presented for discussion before final adoption, allowing team members to provide feedback and adapt to the new approach.
I wouldn't seek additional rules regarding PRs, reversions, or the specific content and results required for approval. Let's proceed based on common sense, assuming that WG members expect a brief overview of the modifications, how they improve the existing tools, and what impact they will have on the current code. I would like to apply the same procedure to any new analyses that introduce new parameters or other collaborative contributions aimed at improving R3BRoot. I'm confident that in this context we will have a productive discussion.

[jose-luis-rs]

Hello @YanzhaoW

1. Under which conditions can a commit be reverted? For this reversion, it was done so quickly even before any conclusion is drawn in NeuLAND WG group. And the PR is merged instantly without allowing any opinions from original submitters.

Your work is still in the PR #1083, but unfortunately I cannot reopen it, this step must be done by @SimonL2718. Sorry for that!

Will it be like this for the future commits from our contributors?

No, really the initial merge was a mistake from my side as I was trying to fix a problem in the ASYEOS repository that might be related to your changes in NeuLAND, but finally the mistake was in the github workflow. After solving the problem with ASYEOS I saw that the merged PR didn't pass the CIs with jan24p1, which is here #1079.

Sorry for the mix-up, to be sure that everything works, please, reopen your PR, I will merge the PR #1079 to start with the CIs under jan24p1 and, after this, rebase the dev branch from your PR to see what happens. After the review by @igasparic et al I will merge it again.

2. If reversion is outside this one week limit. Logically, is it correct to assume the reversion of the reversion also has no such one week limit?

Initially the reversion was done for review by your WG and I don't know how much time they will need for this task. Maybe they suggest changes to the code, ...
Do you need to merge the PR urgently?

[YanzhaoW]

No, merging is not urgent.

Is it ok that I push a PR, which reverts the reversion in this PR? Then we merge the PR after analysis meeting when I present the code changes as requested. The original submitter Simon has finished his bachelor program and I don't think he has any time now to further work on it. If there is something else I need to change, I will push additional PRs for those changes.

[YanzhaoW]

Hi, @hapol Thanks for your response.

In this instance, the reversion was made in response to a WG request, so that the modifications could be discussed further within the WG or during an analysis meeting. This is not a new requirement, but rather a reasonable procedure for teamwork. Several previous major changes have followed a similar process, where modifications were presented for discussion before final adoption, allowing team members to provide feedback and adapt to the new approach.

Sure, I agree that feedbacks and discussions are important. Probably we could utilize the comment section under each PR as well? It's better for the book-keeping and better integrated with git and code changes. The original PR stayed open for nearly a month. If Igor is the maintainer for NeuLAND code, maybe we could put him as a reviewer or an assignee for any PRs relating to NeuLAND. There is also a setting in Github, which only allows the PR to be merged after the permissions from all reviewers.

[hapol]

Hi @YanzhaoW,
yes, we should use the comments as you suggest.
Regarding the options for merging the PR, I would prefer to add maintainers or previous contributors as reviewers, but I wouldn't make their approval a requirement for merging the PR in general. I believe the person submitting the PR should have the discretion to choose the reviewers and decide whether their approval is needed.

[jose-luis-rs]

Hi @YanzhaoW
Yes, please, go ahead with the new PR.
Thanks in advance