PWGJE: First draft of exploratory track skim#3590
Closed
raymondEhlers wants to merge 1 commit intoAliceO2Group:masterfrom
Closed
PWGJE: First draft of exploratory track skim#3590raymondEhlers wants to merge 1 commit intoAliceO2Group:masterfrom
raymondEhlers wants to merge 1 commit intoAliceO2Group:masterfrom
Conversation
raymondEhlers
requested review from
alibuild,
ddobrigk,
maoyx,
mfasDa and
nzardosh
as code owners
Collaborator
|
Hi Raymond, This is very nice but it seems to be the same skim and strategy that was reported a few weeks ago by me and jochen and which is currently in development (the part for charged tracks and collisions in data and MC is done and we are working on EMCAL and HF additions before releasing it). If I am misunderstanding the purpose let me know but if not should we wait till that one is committed (should be a few weeks) and then we can review if it needs any additions, just so we avoid having multiple skims from the JE side? Thanks, |
Collaborator
|
Error while checking build/O2Physics/o2 for 2c7fd43 at 2023-10-15 12:22: Full log here. |
Collaborator
Author
|
Hi Nima, Thanks for your comment! I think our aim here is a little different, but I could have misunderstood your and Jochen's developments - I'm just working off the slides and minutes, so there's definitely lots of space for misunderstanding! My understanding is that you and Jochen were working on an approach to convert AO2D -> slimmed down track skim that would have enough information for everyone to run jet analyses on, but that didn't reference the original AO2D. Is that right? Our goal here was to try to skim down even further, looking at how minimal we can go and how well we can work with these on local, smaller computing resources. Explicit in this would be trading reduced flexibility for reduced storage. I know this has been discussed in the past, but since this approach has worked well for Berkeley folks, I think starting from a familiar approach could be a good entry point for us to be better involved. It ultimately may not be the way that we proceed, but I think this is one of those instances where even a few test trees could be helpful. I labeled as the track skim as LBL since it's experimental at this point, but of course am happy to collaborate/share/drop/whatever as approriate if the goals align. My plan was that this could modified to run on the skims that are produced by you and Jochen's developments, but based on my understand of O2 functionality, I'm not aware of any other way to eg. reduce precision without writing a separate task. All that being said, please correct me if I've gone wrong somewhere! Thanks and have a good weekend! Best, |
Collaborator
|
Hi Raymond, maybe we could set up a meeting early next week to discuss further on zoom. I think the idea is basically the same as we also only keep what is needed in order to be able to run a self contained jet finder, so the goal should be reasonably similar. Its probably best to avoid two parallel developments of the same thing and also to avoid down the line running essentially the same skims twice over the full AOD. If the skims we produce have too much info for you you can of course skim them further to run locally but i guess its best to have one skim over the full AOD that is inclusive of both use cases. I would be happy to meet for example on Tuesday to take a look together? Have a nice weekend too :) |
Collaborator
Author
|
Sure, let's meet and figure out a plan. I'm at the INT workshop this week, but I'm sure we can find some time. Let's try to sort it on mattermost Thanks, Raymond |
nzardosh
previously approved these changes
Oct 20, 2023
Collaborator
|
Error while checking build/O2Physics/o2 for 3622c5f at 2023-10-27 20:56: Full log here. |
raymondEhlers
force-pushed
the
lbl-skim
branch
from
3622c5f to
bf4bba9
Compare
Collaborator
Author
|
@nzardosh When you have a chance, could you please review/approve? All changes + compiler + bug fixes should be in now. Thanks! |
Collaborator
|
Error while checking build/O2Physics/o2 for bf4bba9 at 2023-11-08 13:30: Full log here. |
raymondEhlers
force-pushed
the
lbl-skim
branch
from
bf4bba9 to
3a035fa
Compare
Collaborator
|
Error while checking build/O2Physics/o2 for 3a035fa at 2023-11-09 17:49: Full log here. |
Collaborator
|
Error while checking build/O2Physics/o2 for 0a5dd32 at 2023-11-09 19:07: Full log here. |
Collaborator
|
Error while checking build/O2Physics/o2 for ee0f81a at 2023-11-09 22:59: Full log here. |
raymondEhlers
force-pushed
the
lbl-skim
branch
from
ee0f81a to
94a2945
Compare
Collaborator
|
Error while checking build/O2Physics/o2 for 94a2945 at 2023-11-09 23:28: Full log here. |
Collaborator
|
Error while checking build/O2Physics/o2 for 46f9e35 at 2023-11-15 02:13: Full log here. |
raymondEhlers
force-pushed
the
lbl-skim
branch
from
46f9e35 to
5d41c16
Compare
Collaborator
|
Error while checking build/O2Physics/o2 for 5d41c16 at 2023-11-15 03:42: Full log here. |
Collaborator
|
Error while checking build/O2Physics/o2 for c1a7cee at 2023-11-15 21:51: Full log here. |
Collaborator
|
Hi Raymond, thanks for the updates! In the current configuration what is the difference between your task and the jetderiveddataoutputwriter? I am adding now switches in the writer to select which tables (like BCs or clusters) that one really wants to save, so the only difference in this case would be that you have slightly fewer fields per table and that you also do a very particular track or event selection up front instead of storing a bit? As we discussed before I am very happy to approve this for testing but due to the similarities it might be hard to justify running it over the full sample. However I guess your task can run over the JE derived data sets without a problem and you can then store this even further reduced output starting from those? This could be quite a nice test and if small enough something anyone wanting to do local analysis could use. For this use case I would not forsee a problem if the output sizes are good but it would require all dependency on the original AOD tables to be dropped. Would that be a good approach in your mind? probably its what you intended anyways with the PR? |
Collaborator
Author
|
Hi Nima, Thanks for your comment - a few responses:
I think undersells the benefits a bit:
Yes, that's what I've had in mind
Unless I've overlooked something, I don't think there is any dependence on the AOD tables anymore? If I've missed something, can you please let me know? Or is there somehow an additional issue with calling Best, |
Collaborator
|
Hi Raymond, The problem is that you have an index to Collisions instead of JCollisions as I commented in the PR. I think if this is fixed then it would be independent of the original AO2D and would work well. Thanks, |
raymondEhlers
force-pushed
the
lbl-skim
branch
from
224de71 to
ac5aa87
Compare
Collaborator
Author
|
Hi Nima, Sorry, I'm quite confused - I think your review on the PR was revised and addressed quite a while ago. I just rebased to squash commits, but I made changes to adapt to the new framework (including Collision -> JCollision) a couple of weeks ago. Currently in the code, I see: Can you please highlight the particular line where you see the issue? I don't mean to be difficult - I just don't see it Thanks, |
nzardosh
previously approved these changes
Nov 30, 2023
| namespace ExploratorySkim | ||
| { | ||
| // NOTE: We don't want a Collision Index ID, as this will require the full input AO2D, which we don't want! | ||
| DECLARE_SOA_INDEX_COLUMN(Collision, collision); |
Collaborator
There was a problem hiding this comment.
Why not use an index to JCollisions instead?
Collaborator
Author
There was a problem hiding this comment.
Thanks for pointing this out - it's fixed now. Sorry to have overlooked this - I think you might have posted the commit as part of a review, but the overall review may not have gotten posted until we chatted. In any case, should be resolved now, and thanks for your help!
raymondEhlers
force-pushed
the
lbl-skim
branch
2 times, most recently
from
7e9ab20 to
31cda3a
Compare
Collaborator
|
Error while checking build/O2Physics/o2 for 31cda3a at 2024-02-10 12:47: Full log here. |
|
This PR has not been updated in the last 30 days. Is it still needed? Unless further action is taken, it will be closed in 5 days. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the first draft of a minimal track skim approach to access what can be done feasibility. The current plan is get this running at a small scale, see what can be done to make it feasible, and then look to contribute from there. It's a small step, but we hope an entry point towards further involvement