Introduce run time switch for dN/dx calculation

[plexoos]

The functionality to calculate dN/dx is already in the code. In order to enable it one just need to define an environment variable STAR_USE_DNDX before starting processing jobs.

[plexoos]

@yfisyak Do you have any objections to this proposed change?

[kehw]

Such switches are usually implemented as bfc chain options, right? I am afraid an environment variable based switch won't be as visible as the chain options in log files.

[plexoos]

Perhaps you are right. To be honest I don't have much experience with bfc chain option implementation. If you know how to do it quickly please submit here

[kehw]

I do not know the background of this PR. Is it for some quick fix?
There was a similar discussion in S&C management meeting serval weeks ago. If I understand the discussion right, people also suggested that some infrastructure code is needed around the dNdx calculation.

[plexoos]

The background is very simple. The dNdx feature is enabled by default in the code (essentially hard coded). The dNdx feature is not going to be used in production jobs and has to be disabled BUT the main branch is requested to have the feature on. We either provide a switch or produce a branch with a patch turning the feature off for releases. The latter smells unprofessional to me. Also, the tagging for new release has to happen now so, yes, if you can help act now.

I don't understand the second part of your comment at all... What infrastructure code needed? Any pointers to the discussion?

[kehw]

For an immediate action, I have no problem with code change here. Gene needs to comment if the production team is ok to work with a new environment variable.
I cannot find the minutes of the meeting I mentioned, but it was about whether we need a chain option for dNdx.

[veprbl]

Change looks fine apart from that it changes the default behaviour

[plexoos]

Change looks fine apart from that it changes the default behaviour

No worries. There was an email (see star-scmgt-l) that the default must be set to off. The "STAR Management" was mentioned four times in that email so, it must mean something 😄

[plexoos]

@starsdong Xin, if you want to follow up on this change please do now. You can also approve it and merge. Thanks

[genevb]

I have no objections to the proposed changes under StRoot/StBFChain. The github interface seems to provide no way for me to approve only those specific changes in this larger pull request, and I abstain from review of the other proposed code changes to the main branch. Further, I see that the github interface is forcing me to be listed as a reviewer because I'm listed as a CODEOWNER for just a portion of the subdirectories involved in this pull request and it will no longer allow me to decline the role. So I will just have to leave this as a comment and leave it at that.

[plexoos]

Xin, if you want to follow up on this change please do now. You can also approve it and merge. Thanks

Xin, yesterday after my comment above you replied to me privately that you were still discussing this change with the STAR management. Is there any news on this? Can we merge already?

[plexoos]

Ping @starsdong

[starsdong]

Dmitri, would you please update the CODEOWNERS file to have Yuri F. as the owner for the dEdxY2Maker? It is preferred to have approvals from the code owners for all PRs. I hope we can conclude on this early next week.

[plexoos]

Is there a problem for Yuri to do this himself? He is welcome to create a PR with changes to CODEOWNERS

[starsdong]

Hi All,

After discussion with Yuri who is the code owner of dEdxY2Maker, Yuri proposed a different approach to use SkipdNdx option in the chain. See pull request
#52

If OK with everyone, I would suggest reviewers to take a look at the above #52 request and proceed from there?

Thanks

/in

[klendathu2k]

Hi Xin, Both solutions (PR #36 and #52) allow the decoupling of the dE/dx and dN/dx algorithms. PR #36 makes the decoupling the default, PR #52 requires a specific chain option to be added to decouple the two. With PR #36, the "dEdxY2" option reverts back to what it meant prior to the inclusion of dN/dx in the dEdx maker (2016-09-18). Specifically, run the dEdxY2 algorithm. With PR #52, the "dEdxY2" option maintains its meaning from the point where dNdx was added: run both algos. Neither solution is ideal here, because they *both* change the behavior of production chains. But there are a couple of reasons to prefer PR #36. 1) dN/dx has never been used in published results. Dropping it from the production chains is less problematic. 2) dN/dx adds a CPU overhead. PR #52 requires all users to add "skipDnDx" to their chain to avoid this. Cheers, Jason

…

On 2021-07-02 16:00, Xin Dong wrote: Hi All, After discussion with Yuri who is the code owner of dEdxY2Maker, Yuri proposed a different approach to use SkipdNdx option in the chain. See pull request #52 [1] If OK with everyone, I would suggest reviewers to take a look at the above #52 [1] request and proceed from there? Thanks /in -- You are receiving this because your review was requested. Reply to this email directly, view it on GitHub [2], or unsubscribe [3]. Links: ------ [1] #52 [2] #36 (comment) [3] https://github.com/notifications/unsubscribe-auth/ANL4LVAYA6DWFVHWNJ27TJLTVYLEVANCNFSM464QSOCA

[plexoos]

I agree with Jason. #36 should be preferred over #52 because according to PWGC there is no immediate interest in using dNdx. Again, the foreseen production jobs will not include dNdx, therefore, enabling it by default just to use the "skipDnDx" option to turn it off looks strange.

[plexoos]

Close in favor of #52. The difference between this and #52 is the default value of the switch turning the dNdx calculation on/off

impl1

impl2