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

Remove StRoot/StShadowMaker and "shadow" option #153

Closed
wants to merge 1 commit into from

Conversation

plexoos
Copy link
Member

@plexoos plexoos commented Sep 22, 2021

This code cannot be used so I propose to remove it. Alternatively, the authors could fix the implementation by providing the valid code in StRoot/StShadowMaker but my understanding from #136 is that they are fine with just removing it.

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.

My understanding is that this is no longer my responsibility.

@plexoos
Copy link
Member Author

plexoos commented Sep 22, 2021

As you wish. You can remove the corresponding entry from the CODEOWNERS file in order to avoid automatic notifications

/StRoot/StShadowMaker @genevb

@klendathu2k
Copy link
Contributor

That is not quite my reading of the discussion in #136.

StShadowMaker was used in the blinding/unblinding of data for the isobar data. That is the reason that the implementation file is encrypted. This code is (I believe) needed in order to be able to read / process the isobar data, and thus we need to maintain this in our library releases. Now that the data has been unblinded, it is not clear to me that StShadowMaker should remain encrypted. But that decision should be taken up as a topic for an S&C meeting.

@plexoos
Copy link
Member Author

plexoos commented Sep 22, 2021

Just like I said:

Alternatively, the authors could fix the implementation by providing the valid code in StRoot/StShadowMaker

Who is the author? Jerome? @jlauret

@jlauret
Copy link

jlauret commented Sep 22, 2021

This code does not need to be deployed (it is not part of the chain except the blind analysis). We can leave it encrypted and never use in a checkout. Only the header can / should be deployed.
Furthermore, I no longer have responsibility for the fate of any code in STAR remember? So, while the suggestion is already to leave it "as-is" and not checkout upon new library release (only the header file is needed, not the code itself), @genevb should IMO be the new owner of this piece of code.

@genevb
Copy link
Contributor

genevb commented Sep 22, 2021

A couple things that appear to need clarifying:

  • StShadowMaker is not needed to read the unblinded data, which is what is now in the hands of analyzers. It is only necessary for blinding purposes.
  • Jerome & I were jointly involved in preparing the StShadowMaker code and encrypting it. If there is a directive from the Collaboration (particularly the Analysis Blinding Committee) to provide it unencrypted, this is of course possible.

EDIT: I see Jerome responded at about the same time I did, so there is some overlap in what we have said.

@plexoos
Copy link
Member Author

plexoos commented Sep 22, 2021

We can leave it encrypted and never use in a checkout.

Why? It cannot be compiled, it cannot be used.
Why do you want to complicate the life of our users by requiring a sub-standard clone procedure? I hope you do realize that the production team is not the only user who can clone and compile the entire repo?

@klendathu2k
Copy link
Contributor

klendathu2k commented Sep 22, 2021 via email

@jlauret
Copy link

jlauret commented Sep 22, 2021

Alternative to removing it: modify cons to ignore by default.

Or rename .cxx.enc and move on. So many possibilities ...

@plexoos
Copy link
Member Author

plexoos commented Sep 22, 2021

modify cons to ignore by default.

Exactly my proposal #69 since July 26

@genevb
Copy link
Contributor

genevb commented Sep 22, 2021

As you wish. You can remove the corresponding entry from the CODEOWNERS file in order to avoid automatic notifications

/StRoot/StShadowMaker @genevb

My understanding != my wish
And apparently CODEOWNERS != responsibility for maintaining code (this point was made in #69 )

@plexoos
Copy link
Member Author

plexoos commented Sep 22, 2021

My understanding != my wish

It's up to you

And apparently CODEOWNERS != responsibility for maintaining code (this point was made in #69 )

Your point != policy adopted by the collaboration. If there is a document describing the procedure of who and how can modify the code then please let us know.

@plexoos plexoos closed this Oct 11, 2021
@plexoos plexoos deleted the pr/rm_shadowmaker branch October 19, 2021 19:34
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.

None yet

4 participants