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 unsupported chars from PPSAlignmentWorker #38928

Closed
wants to merge 1 commit into from

Conversation

tvami
Copy link
Contributor

@tvami tvami commented Aug 1, 2022

PR description:

Quick fix to #38885
PPS experts will follow up when we are less constrained in time with possible optimization (like refactoring the loop)

PR validation:

Ran wf 1042

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Not a backport, but needs to backport to 12_4_X

@tvami
Copy link
Contributor Author

tvami commented Aug 1, 2022

type ctpps,bugfix

@cmsbuild cmsbuild added the urgent label Aug 1, 2022
@tvami
Copy link
Contributor Author

tvami commented Aug 1, 2022

test parameters:

  • workflows = 1042

@MatiXOfficial
Copy link
Contributor

Weird that 1042 passed. You should also update PPSAlignmentHarvester and PPSAlignmentConfigurationESSource.

@tvami
Copy link
Contributor Author

tvami commented Aug 1, 2022

Weird that 1042 passed.

Sorry I dont follow, why is that weird?

@MatiXOfficial
Copy link
Contributor

Because you changed the paths of the plots which are used by the harvester. If there is no error, then I assume that the results are empty.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 1, 2022

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38928/31374

  • This PR adds an extra 12KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@MatiXOfficial
Copy link
Contributor

MatiXOfficial commented Aug 1, 2022

The worker encodes some numbers in the paths, that's why there were points in the first place. If they are replaced by 'p', the harvester cannot properly decode them as doubles.

@tvami
Copy link
Contributor Author

tvami commented Aug 1, 2022

So I learned @MatiXOfficial actually have made his own version of the solution, thus I'm closing this PR

@tvami tvami closed this Aug 1, 2022
@cmsbuild cmsbuild removed the urgent label Aug 1, 2022
@tvami
Copy link
Contributor Author

tvami commented Aug 1, 2022

-alca

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants