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

understand if SiPixelRecHitConverter can be left unchanged #440

Open
fwyzard opened this issue Feb 11, 2020 · 13 comments
Open

understand if SiPixelRecHitConverter can be left unchanged #440

fwyzard opened this issue Feb 11, 2020 · 13 comments
Labels
Pixels Pixels-related developments

Comments

@fwyzard
Copy link

fwyzard commented Feb 11, 2020

SiPixelRecHitConverter produces a HostProduct<unsigned int[]> even when running a CPU-only, legacy-only workflow, because it needs to produce the very same products as SiPixelRecHitSoAFromLegacy, even if they are not used.

This caused a problem for Phase 2 workflows, which worked around by #438.

We should strive to leave the legacy-only workflow unaffected - or get an explicit agreement from the DPG.

@fwyzard fwyzard added the Pixels Pixels-related developments label Jun 18, 2020
@fwyzard
Copy link
Author

fwyzard commented Nov 28, 2020

Discussed here: cms-sw#31721 (comment) .

@tsusa
Copy link

tsusa commented Dec 1, 2020

@fwyzard @VinInn would it be possible to discuss this issue tomorrow at the pixel offline meeting https://indico.cern.ch/event/934828/#b-382402-pixel-offline-meeting

@fwyzard
Copy link
Author

fwyzard commented Dec 1, 2020

Mhm, tomorrow (Wednesday) is a bit crowded for me, with the HLT meeting 13-15 and the Run 3 preparation 15-17.
I could skip the second part of the Run 3, and join the Tracker DPG around 16:45 ?

@tsusa
Copy link

tsusa commented Dec 2, 2020

The agenda is quite full (last item is scheduled for 17:10), so it should be fine. I can ping you when we are done with the other items.

@VinInn
Copy link

VinInn commented Dec 2, 2020

This is a complicated issue. @makortel may have some ideas.
Anyhow, what is the problem?
From my side It surely cannot be done urgently (say before Xmas)
As I already discussed several time during the previous presentation the four producers of hits should be integrated and share most of the code. My understanding was that Tracker DPG was taking care of this after the integration.

@fwyzard
Copy link
Author

fwyzard commented Dec 2, 2020

Anyhow, what is the problem?

No particular problem on my side, just part of the review of cms-sw#31721 .

@tsusa
Copy link

tsusa commented Dec 2, 2020

@fwyzard @VinInn @makortel As we are targeting 11_3 that timeline should be OK. Let us know if we can discuss it today. If it is too close, let us know if the DPG meeting next Tuesday would be OK.

@VinInn
Copy link

VinInn commented Dec 2, 2020

Is this considered a blocking issue?

@tsusa
Copy link

tsusa commented Dec 2, 2020

if it is a blocking issue, that depends on the PR reviewer's opinion.
At any rate, we'd be happy to learn more in detail about it and discuss with you, even if we shall address it later

@VinInn
Copy link

VinInn commented Dec 2, 2020

I do not know the details. It is a Framework issue.
It was the only way I found to make it working.
I do not have a solution.

@fwyzard
Copy link
Author

fwyzard commented Dec 2, 2020

So, in the GPU workflow

  • process.siPixelRecHitsPreSplitting can be either a SiPixelRecHitFromSOA module or a SiPixelRecHitConverter module
  • for this to work, the two modules must provide the same collections
  • thus, SiPixelRecHitConverter now produces the extra uint32_t[] collection

However, for this workflow, I don't think that collection is used downstream, so we could

  • not add it to SiPixelRecHitConverter
  • "hide it" when running SiPixelRecHitFromSOA using an EDAlias

@VinInn would this work ?

If you think so, I can cook up the EDAlias configuration.

@fwyzard
Copy link
Author

fwyzard commented Dec 2, 2020

On the other hand, if producing the uint32_t[] collection from the SiPixelRecHitConverter module is useful downstream (whether it is currently used or not for the moment) then we should just keep things as they are :-)

@VinInn
Copy link

VinInn commented Dec 2, 2020

I confirm that, at least for the current version of the legacy workflow, the extra collection is not needed.
I suggest to try. (never user EDAlias). I think that was the suggestion by Matti as well.

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

No branches or pull requests

3 participants