search

star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

Introduce run time switch for dN/dx calculation #36

Closed plexoos closed 2 years ago

plexoos commented 2 years ago

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 commented 2 years ago

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

kehw commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

plexoos commented 2 years ago

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 commented 2 years ago

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

plexoos commented 2 years ago

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 commented 2 years ago

Ping @starsdong

starsdong commented 2 years ago

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 commented 2 years ago

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

starsdong commented 2 years ago

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 https://github.com/star-bnl/star-sw/pull/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 commented 2 years ago

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] https://github.com/star-bnl/star-sw/pull/52 [2] https://github.com/star-bnl/star-sw/pull/36#issuecomment-873229077 [3] https://github.com/notifications/unsubscribe-auth/ANL4LVAYA6DWFVHWNJ27TJLTVYLEVANCNFSM464QSOCA

plexoos commented 2 years ago

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 commented 2 years ago

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