Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

PWGHF: Implementation of D-Dbar generic correlation task and related skimmers (For D0-D0bar and Dplus-Dminus)#6124

Merged
ktf merged 17 commits intoAliceO2Group:devAliceO2Group/AliceO2:devfrom
fcolamar:FC_DplusDminusfcolamar/AliceO2:FC_DplusDminusCopy head branch name to clipboard
Jun 9, 2021
Merged

PWGHF: Implementation of D-Dbar generic correlation task and related skimmers (For D0-D0bar and Dplus-Dminus)#6124
ktf merged 17 commits intoAliceO2Group:devAliceO2Group/AliceO2:devfrom
fcolamar:FC_DplusDminusfcolamar/AliceO2:FC_DplusDminusCopy head branch name to clipboard

Conversation

@fcolamar
Copy link
Contributor

Note: it's an alternative pull request w.r.t. the one currently open (#5450), which implemented only D0D0bar correlations.
Either we approve this one (more recent and complete), or the other (which was partially reviewed)

@fcolamar
Copy link
Contributor Author

@ginnocen, @vkucera, it's an alternative pull request w.r.t. the one currently open (#5450), which implemented only D0D0bar correlations. So if this version is approved (which is more recent and complete, though not tested on a full sample), the other PR (which was partially reviewed) can be discarded

@vkucera
Copy link
Collaborator

vkucera commented May 25, 2021

@fcolamar Your branch is quite old. Could you please sync it with dev so that we run the latest tests and make sure that there are no conflicts?

Analysis/Tasks/PWGHF/CMakeLists.txt Outdated Show resolved Hide resolved
Analysis/Tasks/PWGHF/D0D0barCorrelator.cxx Outdated Show resolved Hide resolved
Analysis/Tasks/PWGHF/D0D0barCorrelator.cxx Outdated Show resolved Hide resolved
Analysis/Tasks/PWGHF/D0D0barCorrelator.cxx Outdated Show resolved Hide resolved
Analysis/Tasks/PWGHF/D0D0barCorrelator.cxx Outdated Show resolved Hide resolved
Analysis/Tasks/PWGHF/taskDDbarCorrelation.cxx Outdated Show resolved Hide resolved
Analysis/Tasks/PWGHF/taskDDbarCorrelation.cxx Outdated Show resolved Hide resolved
Analysis/Tasks/PWGHF/D0D0barCorrelator.cxx Show resolved Hide resolved
Analysis/Tasks/PWGHF/D0D0barCorrelator.cxx Show resolved Hide resolved
@fcolamar
Copy link
Contributor Author

@ginnocen @vkucera thanks, I updated the PR according to your comments. Also merged with the updates from the dev (this I did yesterday though)

Analysis/Tasks/PWGHF/taskCorrelationDDbar.cxx Outdated Show resolved Hide resolved
@fcolamar
Copy link
Contributor Author

fcolamar commented Jun 2, 2021

After running the code on the updated Run3VF (thanks to @vkucera for the assistence in introducing the alternate running modes), I tested the new version of the code and did a couple of minimal fixes and improvements, which do not alter anyway its structure in any way. All the above points from Vit's review were also discussed/implemented, so the code should now be in its final version (at least for performing the steps currently implemented)

@ginnocen
Copy link
Collaborator

ginnocen commented Jun 2, 2021

hi @vkucera @fcolamar, I would suggest we merge it, unless there are major issues. It is in any case a code that will evolve over time. At this stage If we want to try to get a performance plot for the workshop we need it in the tag asap...

Analysis/Tasks/PWGHF/HFCorrelatorD0D0bar.cxx Outdated Show resolved Hide resolved
vkucera
vkucera previously approved these changes Jun 3, 2021
@vkucera vkucera marked this pull request as ready for review June 3, 2021 09:14
@fcolamar
Copy link
Contributor Author

fcolamar commented Jun 4, 2021

Hi all, from the above logs I'm actually not able to get the reason of the compilation errors (also, on my PC everything compiles smoothly), do you have any hint?

@vkucera
Copy link
Collaborator

vkucera commented Jun 5, 2021

Hi all, from the above logs I'm actually not able to get the reason of the compilation errors (also, on my PC everything compiles smoothly), do you have any hint?

The logs look quite clear to me:

  • build/O2/fullCI failed because you are missing readability-braces-around-statements.
  • build/O2/o2 was killed because of memory usage.

@fcolamar
Copy link
Contributor Author

fcolamar commented Jun 7, 2021

Hi all, from the above logs I'm actually not able to get the reason of the compilation errors (also, on my PC everything compiles smoothly), do you have any hint?

The logs look quite clear to me:

  • build/O2/fullCI failed because you are missing readability-braces-around-statements.
  • build/O2/o2 was killed because of memory usage.

Hi Vit, that's strange, for build/O2/fullCI the log I had seen was very different and more obscure when I checked, with respect to the current (it had also errors at the level of Lc and D+ selectors, with no additional info, from what I remember), this is not clear to me. Anyway, I've fixed the readability error from the current log, those were indeed trivial. What do I have to do for the build/O2/o2 error instead?

@vkucera
Copy link
Collaborator

vkucera commented Jun 7, 2021

Hi all, from the above logs I'm actually not able to get the reason of the compilation errors (also, on my PC everything compiles smoothly), do you have any hint?

The logs look quite clear to me:

  • build/O2/fullCI failed because you are missing readability-braces-around-statements.
  • build/O2/o2 was killed because of memory usage.

Hi Vit, that's strange, for build/O2/fullCI the log I had seen was very different and more obscure when I checked, with respect to the current (it had also errors at the level of Lc and D+ selectors, with no additional info, from what I remember), this is not clear to me. Anyway, I've fixed the readability error from the current log, those were indeed trivial. What do I have to do for the build/O2/o2 error instead?

Hi Fabio, I don't think you can do anything about the memory usage, unless it is caused by your code (which I guess is not the case). It's a persistent problem with the resources available to the test builds.

@vkucera
Copy link
Collaborator

vkucera commented Jun 8, 2021

@jgrosseo @iarsene The errors seem unrelated.

@ktf ktf merged commit da396e5 into AliceO2Group:dev Jun 9, 2021
@fcolamar fcolamar deleted the FC_DplusDminus branch June 9, 2021 06:16
EmilGorm pushed a commit to EmilGorm/AliceO2 that referenced this pull request Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.