fixed a logical error for the PicoVtxMod::Mtd in StPicoDstMaker.cxx

[techuan-huang]

I fixed a logical error for the PicoVtxMod::Mtd in StPicoDstMaker.cxx

Origianl:

1. Select default vertex first
2. Select each vertex in the vertex loop in order to find the vertex with at least 2 MTD matched primary tracks
3. Select MTD vertex if it is found -> If the MTD vertex is not found (most of the cases), the vertex loop will continue to the end and the last one is selected.

Fixed:

1. Instead of selecting the vertex directly, I store the default vertex index in a variable VtxIndex.
2. VtxIndex will be changed if MTD vertex is found in the vertex loop.
3. Select the vertex after the vertex loop. -> Default vertex will be selected if no MTD vertex is found in the loop.

Detail can be found at: https://drupal.star.bnl.gov/STAR/system/files/TeChuan_Run17_SL21c_vtxQA.pdf

[plexoos]

Very nice catch and fix. However, you should not modify the indent to 4 spaces if 2 spaces are used everywhere in the file.

[techuan-huang]

Very nice catch and fix. However, you should not modify the indent to 4 spaces if 2 spaces are used everywhere in the file.

Dmitri, thanks for the comments. I changed it back to 2 spaces indent.

[genevb]

My understanding is that we will want to patch SL21c with this when it's ready. The alternative is to just wait and include it in the SL21d library that we're looking to freeze from main sometime in the next couple weeks, but that implies delaying the Run 17 pp510 st_mtd production until SL21d is ready. My preference is to update SL21c to avoid more-uncertain timelines for SL21d readiness. Any comments on this point, @marrbnl , @starsdong , or @plexoos ?

[marrbnl]

@genevb I agree that patching SL21c is a better way to proceed. It does not interfere with anything else

[plexoos]

@genevb It appears that this change will result in different vertices in PicoDst files. Is the plan to substitute the already released SL21c libraries (i.e. rebuild) with the ones built from this code? Wouldn't this cause confusion for analyzer when they refer to or use the SL21c release in the future?

[marrbnl]

The affected vertex algorithm has not been used in any Run17 data production so far. It is designed specifically for st_mtd stream. So we will need to rebuild the library with the fix, and proceed with the st_mtd production.

[plexoos]

Ok, fine. Thanks for dispelling my second-order concerns. These would not exist if each library release would correspond strictly to one tag. In STAR, however, a SL21c library is built from SL21c_3 tag today and from SL21c_4 tomorrow...

@techuan-huang Could you update your first post with a summary from your slide 11? It summarizes the issue and solution nicely and will go into the log message. You can keep the link to the slides of course if you want.

[techuan-huang]

Thanks for the suggestion. I modified the first post.

[genevb]

On Oct 6, 2021, at 5:13 PM, Dmitri Smirnov @.**@.>> wrote:

Ok, fine. Thanks for dispelling my second-order concerns. These would not exist if each library release would correspond strictly to one tag. In STAR, however, a SL21c library is built from SL21c_3 tag today and from SL21c_4 tomorrow...

I appreciate that you're being careful about this, Dmitri, but you don't have to be concerned about it unless you don't trust others to worry over these concerns. The Production Team's responsibilities include the library releases and we do not intend to update SL21c with versions that would alter the results of past or ongoing official P21ic productions. Maybe it would help you if we explain the versioning approach and summarily review the versioning of SL21c to date:

SL21c => SL21c_1 : updates for simulations for embedding for SL21c that hadn't been used yet (and thus important to keep the reconstruction identical to SL21c), along with changing a chain option name that hadn't yet been used

SL21c_1 => SL21c_2 : update for a chain option that hadn't been used yet

SL21c_2 => SL21c_3 : this change could have warranted a new library, as it was a bug fix, but the evidence seemed to indicate that the bug hadn't yet impacted any official productions yet (we did discuss advancing to SL21d at this point)

SL21c_3 => SL21c_4 : update for a chain option that hasn't been used yet in an official production (test production only)

A user will only ever need to have access to the last version of SL21c and it should be usable for all purposes related to P21ic productions (though we have in the past made "embedding" libraries when the versioning of the reconstruction library had solidified but updates for embedding touched some core codes, in which case the embedding version should suffice for all purposes of an official production library). Conversely, if we kept each and every one of these as separate libraries that are always available on disk because one production or another used each one, we would eat up a considerable amount of space with the growing number of provided libraries, and you are already aware that the space we have has limitations.

That said, we do also sometimes advance the letter version even when there aren't necessarily any impacts relevant to past productions when there is significant code development (e.g. new subsystem, or new and not yet used features in existing codes). There have been past years where code developments were more rapid than they are today and we've seen ~10 letter versions. Maybe we'll end up with 5 or 6 letter versions in 2021 (I expect we should see updates relevant to both mid-rapidity tracking [CA] and forward tracking at some point after SL21d).

-Gene

[genevb]

I'll also add that the attention given by the Production Team regards proposed patches to official libraries; we are not paying attention to every code change coming through git, which is the responsibility I understand the PR reviewers have taken up for the Collaboration. The Production Team has nevertheless historically attempted to summarize the changes we see between letter versions of libraries for the Collaboration, but I'd be open to discussing changing who takes that responsibility if someone else wants to, or thinks they should do it.

-Gene

[plexoos]

Hi Gene,

Yes, there are different types of changes one can make to the code. Different changes affect the functionality in different ways. For this particular change, only an expert can tell that this modified piece of code was never executed in any of the officially ran processing chains and we should not worry about reproducibility. That's what I wanted to confirm and received a definitive answer. On the other hand, generally speaking, no one can guarantee that someone had not used the published libraries and executed the previous revision of the code. This is where I slightly disagree with this assumption "A user will only ever need to have access to the last version of SL21c"

If you want to discuss how to reduce the size of STAR releases, let's do that somewhere else. I might have some ideas.

The list of log messages that the production team has maintained is not very useful in my opinion. It can be generated automatically with a single command from the history of changes. Usually, projects maintain a CHANGELOG file with human friendly list of changes with emphasis on the important ones.

[genevb]

Hi, Dmitri

I slightly disagree with this assumption "A user will only ever need to have access to the last version of SL21c"

This is a goal, not an assumption of the Production Team, with the intention that for a general user it is an assumption they can make. I do not dispute that there will be an exception here or there that escapes us.

If you want to discuss how to reduce the size of STAR releases, let's do that somewhere else. I might have some ideas.

I'll listen to your ideas.

Usually, projects maintain a CHANGELOG file with human friendly list of changes with emphasis on the important ones.

I suggest you present your CHANGELOG plan to S&C Management and we go from there.

Thanks, -Gene

[plexoos]

I added this fix to SL21c. New libraries can be built from SL21c_4 tag.