Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create PWGTools area #340

Merged
merged 9 commits into from
Mar 24, 2023
Merged

Create PWGTools area #340

merged 9 commits into from
Mar 24, 2023

Conversation

nigmatkulov
Copy link
Member

The commit will create a new directory that should be excluded from the compilation process. At the first iteration it should be cross checked (if compilation will pass the CI/CD). Also before merging Zachary Sweger should confirm that the subdirectories contain the latest versions of the Glauber codes.

@veprbl
Copy link
Member

veprbl commented Apr 26, 2022

A question about naming. Does "PWG" stand for "Physics Working Group"? Which one is referred to? Also, if this is a collection of libraries, why it doesn't follow the standard St*Pool naming?

@nigmatkulov
Copy link
Member Author

A question about naming. Does "PWG" stand for "Physics Working Group"? Which one is referred to?
To all PWGs.

Also, if this is a collection of libraries, why it doesn't follow the standard St*Pool naming?
I think it is doable, but idea was to create an area where PWGs could store all physics-related codes,
not software/hardware software which used for the library builds. At some point it may be deployed
from StRoot.

@plexoos
Copy link
Member

plexoos commented Apr 27, 2022

Would it make sense to keep these files in a separate repository? I thought that idea was discussed in the meetings but I don't remember the arguments against it

@marrbnl
Copy link
Member

marrbnl commented Apr 27, 2022

Originally, I was worried about synchronization to RCF if in a separate repository. Thinking more on this, putting these codes in a separate repository does come with the benefit of decoupling from StRoot, so it might be easier to manage and work with. The downside is that people need to download it from Git as it won't be available on RCF, if I understand correctly

@plexoos
Copy link
Member

plexoos commented Apr 27, 2022

I believe what you really want to share on the SDCC machines is the libraries built from the source code rather than the source code itself. But if the code cannot be compiled then there will be no libraries to share. I understand there are some scripts to run jobs that can be also shared, but again, installing them without something they can use wouldn't make sense either.

@marrbnl
Copy link
Member

marrbnl commented Apr 30, 2022

I was actually thinking of sharing the source codes and running scripts, basically the whole package. I think I am used to CVS, where I just copy over the source codes. In any case, I am fine with a separate repository

@marrbnl
Copy link
Member

marrbnl commented May 11, 2022

Hello. To follow up on this, what is the next step?

@veprbl
Copy link
Member

veprbl commented May 11, 2022

If I were a reviewer, I would suggest:

  • Rename PWGTools to StCentralityPool
  • Put makers at the root of that directory, deduplicate StCentralityAnalyzer (we don't need 3 copies of it)
  • Put ROOT macros and shell scripts in StCentralityPool/StFooBarMaker/macros

@marrbnl
Copy link
Member

marrbnl commented May 11, 2022

I prefer the name PWGTools since more codes could be saved in this directory, e.g. codes used to evaluate tracking efficiency uncertainty, etc. Meanwhile, I agree that we should not commit duplicated codes, and a dedicated macro directory sounds like a good idea.

Thinking about how we might use this directory in the future, a separate repository does make sense.

@plexoos
Copy link
Member

plexoos commented May 11, 2022

  • If a separate repository is desired under the star-bnl organization please propose a name. Alternatively, just create a repository under your account and then transfer it to this organization using the button under the repository's Settings tab.

  • If you decide to keep this code under StRoot/ in this repository, someone should make sure it compiles before it can be merged. In order to compile subdirectories the best option would be to follow the naming convention for StXXXPool directories, so cons can apply a special set of build rules for subdirectories as described here: https://drupal.star.bnl.gov/STAR/comp/sofi/soft-n-libs/standards

@marrbnl
Copy link
Member

marrbnl commented May 12, 2022

I think a repository named star-pwg-tools under star-bnl would do.

@plexoos
Copy link
Member

plexoos commented May 12, 2022

Done.

@plexoos
Copy link
Member

plexoos commented May 19, 2022

Closing because star-pwg-tools will be used for this purpose

@plexoos plexoos closed this May 19, 2022
@veprbl
Copy link
Member

veprbl commented May 19, 2022

I think that the creation of side repo under star namespace is unjustified in this case. It has no code review policy or CI in place, and the only apparent reason for its existence is to bypass those requirements.

@marrbnl
Copy link
Member

marrbnl commented May 20, 2022

The main motivation when I suggested to create a separate repository is not to interfere with StRoot, which I always thought is for codes needed for production. The codes we would like to add are more analysis specific with lots of macros and scripts. There was no intention at all to bypass any CI test or code review. If people think it is better to add it to StRoot, that is also fine with me.

@marrbnl
Copy link
Member

marrbnl commented May 20, 2022

I think the key is to make a decision, and move ahead.

@veprbl
Copy link
Member

veprbl commented May 20, 2022

The main motivation when I suggested to create a separate repository is not to interfere with StRoot, which I always thought is for codes needed for production. The codes we would like to add are more analysis specific with lots of macros and scripts. There was no intention at all to bypass any CI test or code review. If people think it is better to add it to StRoot, that is also fine with me.

This is not a question of the motivation. I am only predicting the outcome of creating such separate repo under such circumstance - it will be an absence of the CI and there won't be much code review. The multirepo approach is not forbidden in principle, but it does require some extra dedication to maintain, and it takes more mental effort to track all the different versions of various codes. That's one argument against segregating from the "production" codes, for which all the procedures are already in place. It also should make sense to have all core functionality codes such as reconstruction, calibration and common physics tools in one place where they are most seen by the software experts. Plus, they all serve the same goal of enabling wide range of collaborators to do their best with our data.

@plexoos plexoos reopened this Mar 20, 2023
@plexoos plexoos force-pushed the PWGTools branch 2 times, most recently from 59330f6 to 6ba24fe Compare March 20, 2023 14:13
Copy link
Contributor

@genevb genevb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Responding to the requested review as a "code owner":

I'm not a supporter of adding yet another directory under StRoot that is specifically excluded from compilation, but I won't block this effort as I think it's critical to stop the proliferation of individual private repositories that the use of git in STAR has (expectedly) invigorated. We will just add PWGTools to the SKIP_DIRS list of such directories.

(small addendum: Dmitri's commit to change cons so that it excludes PWGTools only applies when SKIP_DIRS is not defined, but for official library compiles we do indeed have SKIP_DIRS defined, so the exclusion will be via SKIP_DIRS for such compiles.)

…ools.

Old centrality-related codes are moved under the former new subdirectory.
@nigmatkulov
Copy link
Member Author

Responding to the requested review as a "code owner":

I'm not a supporter of adding yet another directory under StRoot that is specifically excluded from compilation, but I won't block this effort as I think it's critical to stop the proliferation of individual private repositories that the use of git in STAR has (expectedly) invigorated. We will just add PWGTools to the SKIP_DIRS list of such directories.

(small addendum: Dmitri's commit to change cons so that it excludes PWGTools only applies when SKIP_DIRS is not defined, but for official library compiles we do indeed have SKIP_DIRS defined, so the exclusion will be via SKIP_DIRS for such compiles.)

Hi Gene,
I believe that it is still an intent to make the codes be a part of compilation. I also dislike the idea of codes being compiled. I'm still trying to figure out what is causing the compilation breaks during the builds. I have an idea but need to test it first.

@veprbl
Copy link
Member

veprbl commented Mar 21, 2023

Can we rename PWGTools -> StCentralityPool ?

Updated README for clarity
Updated refMult histogram to have integer bin edges
Added additional functionality to GetCentrality.C
Added example file for GetCentrality.C
Added Vz-correction example macro with example data
@marrbnl
Copy link
Member

marrbnl commented Mar 23, 2023

This PWGTools directory is intended to host common analysis related codes. I suggest not to change the name. If desired, one change CentralityCalibration to StCentralityPool

@plexoos
Copy link
Member

plexoos commented Mar 23, 2023

Ok, let's keep the originally proposed name. If no more immediate comments or additions, we can merge.

@plexoos plexoos merged commit cef7e84 into main Mar 24, 2023
@plexoos plexoos deleted the PWGTools branch March 24, 2023 21:25
@genevb
Copy link
Contributor

genevb commented Mar 25, 2023

I can imagine that it is probably worth associating specific versions of PWGTools codes with specific library versions (there may, for example, be compilation dependencies), so I will deploy the current version of PWGTools when I release a new library, such as SL23b. I am wondering if people foresee updating libraries like SL23b when updated versions of PWGTools codes are developed?

Example:
Let's say that SL23b is the last library to have some particular variable in the PicoDst, replaced by some better means of determining some physics in SL23c. People analyzing PicoDsts produced in SL23b will need to use the PWGTools codes that have the old variable, so they won't want the latest version of PWGTools from DEV. But it isn't until 4 months after producing data with SL23b that analyzers of that data finish their adjustments to relevant codes in PWGTools. Will we then re-tag a new version of SL23b with the updated PWGTools and deploy that to AFS and CVMFS? Such a deployment isn't a big deal since we won't actually need to recompile anything in the official libraries on AFS and CVMFS. But I would like to understand if this will be the expected procedure, or whether analyzers will be expected to pull the version of PWGTools they are expected to use from github themselves (in which case, why deploy PWGTools in any AFS or CVMFS libraries from the start)?

plexoos added a commit that referenced this pull request Mar 29, 2023
The commit will create a new directory that should be excluded from the
compilation process. At the first iteration it should be cross checked
(if compilation will pass the CI/CD). Also before merging Zachary Sweger
should confirm that the subdirectories contain the latest versions of
the Glauber codes.

---------

Co-authored-by: Grigory Nigmatkulov <nigmaktulov@gmail.com>
Co-authored-by: Dmitri Smirnov <dmixsmi@gmail.com>
Co-authored-by: Zachary Sweger <zsweger@berkeley.edu>
plexoos added a commit that referenced this pull request Mar 29, 2023
The commit will create a new directory that should be excluded from the
compilation process. At the first iteration it should be cross checked
(if compilation will pass the CI/CD). Also before merging Zachary Sweger
should confirm that the subdirectories contain the latest versions of
the Glauber codes.

---------

Co-authored-by: Grigory Nigmatkulov <nigmaktulov@gmail.com>
Co-authored-by: Dmitri Smirnov <dmixsmi@gmail.com>
Co-authored-by: Zachary Sweger <zsweger@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants