search

testdockerwcsim / XTriggerApplication

Other
2 stars 3 forks source link

Feature/cpu and gpu #31

Closed federiconova closed 4 years ago

federiconova commented 4 years ago

runs nhits trigger on CPU and GPU

federiconova commented 4 years ago

Hi

If I remove $(BonsaiInclude) and $(BonsaiLib) from those two lines, the compilation fails for me:

[ozc82499@hepacc03 TriggerApplication] $ make GPU ...

\n*** Making lib/libMyToolsGPU.so **** cp UserTools//.h include/ cp UserTools/Factory/*.h include/ g++  -fPIC -shared  UserTools/Factory/Factory.cpp  -DGPU UserTools/CUDA/CUDA_Unity.o -I include -L lib -lStore -lDataModel -lLogging -o lib/libMyToolsGPU.so -I/usr/local/cuda/include   -L/usr/local/cuda/lib64 -lcudart -lcuda -lcudadevrt -I/opt/ppd/t2k/users/federico_nova/HyperK/HyperK_g410/root/include -I/opt/ppd/t2k/users/federico_nova/HyperK/HyperK_g410/WCSim-Tom-trigger/WCSim/include -L/opt/ppd/t2k/users/federico_nova/HyperK/HyperK_g410/root/lib -lCore -lCint -lRIO -lNet -lHist -lGraf -lGraf3d -lGpad -lTree -lRint -lPostscript -lMatrix -lPhysics -lMathCore -lThread -pthread -lm -ldl -rdynamic -L/opt/ppd/t2k/users/federico_nova/HyperK/HyperK_g410/WCSim-Tom-trigger/WCSim -lWCSimRoot -L ToolDAQ/zeromq-4.0.7/lib -lzmq  -I ToolDAQ/zeromq-4.0.7/include/  -L ToolDAQ/boost_1_66_0/install/lib -lboost_date_time -lboost_serialization -lboost_iostreams -I ToolDAQ/boost_1_66_0/install/include In file included from UserTools/Factory/../BONSAI/BONSAI.cpp:1:0,                  from UserTools/Factory/../Unity.cpp:8,                  from UserTools/Factory/Factory.cpp:1: UserTools/Factory/../BONSAI/BONSAI.h:12:26: fatal error: WCSimBonsai.hh: No such file or directory  #include "WCSimBonsai.hh"                           ^ compilation terminated. make: *** [lib/libMyToolsGPU.so] Error 1

Federico

On 12/03/2020 16:03, Tom Dealtry wrote:

@tdealtry commented on this pull request.

In Makefile https://github.com/HKDAQ/TriggerApplication/pull/31#discussion_r391724244:

+DataModelInclude = $(RootInclude) $(WCSimInclude) $(BonsaiInclude) +DataModelLib = $(RootLib) $(WCSimLib) $(BonsaiLib)

This reverts 441b8c5#diff-b67911656ef5d18c4ae36cb6741b7965 https://github.com/HKDAQ/TriggerApplication/commit/441b8c5e0f1c048dbe341805f5967d9dced7942f#diff-b67911656ef5d18c4ae36cb6741b7965 Is there a reason? Best to put it back I think (BONSAI libs/incs aren't actually required in the DataModel)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#pullrequestreview-373690339, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ2CQPVTDBO6SZEX2J3RHEBTXANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6980 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

federiconova commented 4 years ago

mmm. "git pull origin Application" says I am already up-to-date. This is on

git remote -v origin    https://github.com/federiconova/TriggerApplication.git (fetch) origin    https://github.com/federiconova/TriggerApplication.git (push)  as I am now working off my code. I also have a version with:

git remote -v origin    https://github.com/tdealtry/TriggerApplication.git (fetch) origin    https://github.com/tdealtry/TriggerApplication.git (push) git branch   Application

On 12/03/2020 16:26, Tom Dealtry wrote:

@tdealtry commented on this pull request.

Looks good Federico. Think you're just missing doing a |git pull origin Application| (assuming your |origin| is HKDAQ - see |git remote -v| to check)

In Makefile https://github.com/HKDAQ/TriggerApplication/pull/31#discussion_r391724464:

@@ -84,7 +84,7 @@ lib/libMyTools.so: UserTools// UserTools/* | include/Tool.h lib/libDataModel.s @echo "\n*** Making " $@ "****" cp UserTools//.h include/ cp UserTools/Factory/*.h include/

  • g++ -g -fPIC -shared UserTools/Factory/Factory.cpp -I include -L lib -lStore -lDataModel -lLogging -o lib/libMyTools.so $(MyToolsInclude) $(MyToolsLib) $(DataModelInclude) $(DataModelLib) $(ZMQLib) $(ZMQInclude) $(BoostLib) $(BoostInclude) $(BonsaiLib) $(BonsaiInclude)
  • g++ -g -fPIC -shared UserTools/Factory/Factory.cpp -I include -L lib -lStore -lDataModel -lLogging -o lib/libMyTools.so $(MyToolsInclude) $(MyToolsLib) $(DataModelInclude) $(DataModelLib) $(ZMQLib) $(ZMQInclude) $(BoostLib) $(BoostInclude)

Again, I think best to revert

In Makefile https://github.com/HKDAQ/TriggerApplication/pull/31#discussion_r391725166:

+CUDA_HOME=/usr/local/cuda +#CUDA_HOME=/usr/local/cuda-8.0

Maybe get CUDA_HOME from an environment variable instead of hardcoding? I expect different systems have this in a different place

How to do that?

In UserTools/CUDA/GPUFunctions.h https://github.com/HKDAQ/TriggerApplication/pull/31#discussion_r391730087:

@@ -9,9 +9,9 @@ int test_vertices_initialize(); int test_vertices_execute(); int test_vertices_finalize(); int nhits_initialize(); -int nhits_initialize_ToolDAQ(std::string PMTFile, std::string DetectorFile, std::string ParameterFile);

  • int nhits_initialize_ToolDAQ(std::string ParameterFile,std::vector tube_no,std::vector tube_x,std::vector tube_y,std::vector tube_z, int fTriggerSearchWindow, int fTriggerSearchWindowStep, int fTriggerThreshold, int fTriggerSaveWindowPre, int fTriggerSaveWindowPost);

I'm probably missing something, but why does the nhits algorithm need to know about PMT positions?

It doesn't, but it wants to know how many PMTs are in the tank to compute (together with the dark rate of each) the mean expected number of background hits.

In UserTools/DataOut/DataOut.cpp https://github.com/HKDAQ/TriggerApplication/pull/31#discussion_r391733888:

@@ -14,11 +14,7 @@ bool DataOut::Initialise(std::string configfile, DataModel &data){

m_data= &data;
  • // Log needs m_data

Why have you removed all the logging from this file? If you want to turn it off, change the value of |verbose| in the |DataOutConfig| file for the toolchain you're using. Actually... this may be that you're missing pulling some commits from the head (i.e. branch HKDAQ/Application)

In UserTools/Factory/Factory.cpp https://github.com/HKDAQ/TriggerApplication/pull/31#discussion_r391734914:

@@ -12,9 +12,7 @@ if (tool=="nhits") ret=new nhits; if (tool=="test_vertices") ret=new test_vertices; if (tool=="WCSimReader") ret=new WCSimReader; if (tool=="DataOut") ret=new DataOut; -#ifdef BONSAIEXISTS

Again, this is reverting recent commits

In UserTools/Unity.cpp https://github.com/HKDAQ/TriggerApplication/pull/31#discussion_r391734999:

@@ -1,14 +1,11 @@ -#include "../Build.h"

Again, this is reverting recent commits

In UserTools/WCSimReader/WCSimReader.cpp https://github.com/HKDAQ/TriggerApplication/pull/31#discussion_r391735287:

@@ -110,10 +110,8 @@ bool WCSimReader::Initialise(std::string configfile, DataModel &data){ m_data->IDNPMTs = fWCGeo->GetWCNumPMT(); if(m_data->HasOD) { m_data->ODPMTDarkRate = fWCOpt->GetPMTDarkRate("OD");

  • m_data->ODNPMTs = fWCGeo->GetODWCNumPMT();

Again, this is reverting recent commits

In configfiles/WCSimReaderTest/DataOutToolConfig https://github.com/HKDAQ/TriggerApplication/pull/31#discussion_r391736740:

@@ -1,5 +1,6 @@ verbose 3 -outfilename triggered.root +#outfilename triggered.root +outfilename FILES/triggered.root save_multiple_digits_per_trigger 1

Not everyone has a directory called FILES, so probably revert this

In configfiles/WCSimReaderTest/DataOutToolConfig https://github.com/HKDAQ/TriggerApplication/pull/31#discussion_r391737483:

@@ -1,5 +1,6 @@ verbose 3 -outfilename triggered.root +#outfilename triggered.root +outfilename FILES/triggered.root save_multiple_digits_per_trigger 1

Unless we want to create an extra directory in the repo that output can go into by default?

I think it would be useful, yes

In configfiles/WCSimReaderTest/ToolsConfig https://github.com/HKDAQ/TriggerApplication/pull/31#discussion_r391738353:

\ No newline at end of file +out1 DataOut configfiles/WCSimReaderTest/DataOutToolConfig +reconout ReconDataOut configfiles/WCSimReaderTest/ReconDataOutToolConfig

Not sure what the point of this is, given none of the tools in this chain make any reconstructed quantities?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#pullrequestreview-373690623, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ7JSQ6LIXP7M6V5CD3RHEEKNANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6980 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

git remote -v origin    https://github.com/federiconova/TriggerApplication.git (fetch) origin    https://github.com/federiconova/TriggerApplication.git (push)  as I am now working off my code. I also have a version with: git remote -v origin    https://github.com/tdealtry/TriggerApplication.git (fetch) origin    https://github.com/tdealtry/TriggerApplication.git (push) git branch   Application * feature/BONSAI but no HKDAQ anywhere.

Ahh ok. On the version of the code you have that's most up-to-date (I presume yours) this should work

git remote add upstream https://github.com/HKDAQ/TriggerApplication.git
git pull upstream Application
git push origin feature/CPU_and_GPU
tdealtry commented 4 years ago

It doesn't, but it wants to know how many PMTs are in the tank to compute (together with the dark rate of each) the mean expected number of background hits.

Can you instead pass it just a single int with the number of PMTs (rather than the 3 vectors with x,y,z, positions?) to simplify things?

federiconova commented 4 years ago

Good idea, done.

On 12/03/2020 18:37, Tom Dealtry wrote:

It doesn't, but it wants to know how many PMTs are in the tank to
compute (together with the dark rate of each) the mean expected
number of background hits.

Can you instead pass it just a single |int| with the number of PMTs (rather than the 3 vectors with x,y,z, positions?) to simplify things?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-598354426, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ3JFVJE6HLKH6MYFDTRHETYHANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6980 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

brichards64 commented 4 years ago

whats the current status of this Is the PR finished?

tdealtry commented 4 years ago

@federiconova can comment if it has the complete set of features required But in terms of a code review, it's still got some commits that are reverting changes from previous pull requests (which I think is related to the merge conflict), that need to be fixed

federiconova commented 4 years ago

Hi, yes, sorry, I have to still address all of Tom's comments. This will only happen the week after next one. I was stuck on some linking issue because as I understand it some new things were changed in Tom's branch after I forked from it and now I am not sure how to easily recover them. Will work on it mid-April.

Federico

On 02/04/2020 13:54, Tom Dealtry wrote:

@federiconova https://github.com/federiconova can comment if it has the complete set of features required But in terms of a code review, it's still got some commits that are reverting changes from previous pull requests (which I think is related to the merge conflict), that need to be fixed

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-607827950, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ6EO6EHBYQ2N2LZJFLRKSDGZANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

@federiconova thanks for making these changes!

I started looking through the commits & quickly got confused - you're great at doing confusing things with git! 🙃 Anyway, I thought I'd try get rid of all the commits that other people did, so I can actually review this PR properly - that's worked (just noticed now that you've got merge conflicts, so I should of just asked you to do a pull - never mind). Anyway what you'll need to do is:

git remote add tom git@github.com:tdealtry/TriggerApplication.git
git fetch tom feature/CPU_and_GPU
git pull tom feature/CPU_and_GPU
git push origin feature/CPU_and_GPU #this assumes your origin remote is git@github.com:federiconova/TriggerApplication.git

Obviously check it works after that etc. (SubSample may need changing, but I think I prefer a function like vector<int> SubSample::GetTimesInt() rather than keeping another copy of the times vector in the SubSample itself that needs sorting, etc., and won't be used for CPU code)

Anyway, let me know when that's working & I'll do a proper review again (and ask @ast0815 to take a look at the time book-keeping)

federiconova commented 4 years ago

HI

Indeed I am very creative at making github trouble. But look, I was just following religiously your previous instructions. I forked my branch from  https://github.com/HKDAQ/TriggerApplication and entirely ignored your tdealtry github.

Now I am confused as ever. Why do I have to add your remote etc? And what does it mean that you got rid of other people commits? I just redid my all code against the most up-to-date TriggerApplication master branch.

Your instructions don't work:

[ozc82499@hepacc01 TriggerApplication] $ git branch   Application

Please make sure you have the correct access rights and the repository exists.

Anyway I am lost at what I am trying to do. I made a pull request from my github into the master or whatever it is  called. What is wrong with that?

On 22/04/2020 10:38, Tom Dealtry wrote:

@federiconova https://github.com/federiconova thanks for making these changes!

I started looking through the commits & quickly got confused - you're great at doing confusing things with git! 🙃 Anyway, I thought I'd try get rid of all the commits that other people did, so I can actually review this PR properly - that's worked (just noticed now that you've got merge conflicts, so I should of just asked you to do a pull - never mind). Anyway what you'll need to do is:

|git remote add tom git@github.com:tdealtry/TriggerApplication.git git fetch tom feature/CPU_and_GPU git pull tom feature/CPU_and_GPU git push origin feature/CPU_and_GPU #this assumes your origin remote is git@github.com:federiconova/TriggerApplication.git |

Obviously check it works after that etc. (SubSample may need changing, but I think I prefer a function like |vector SubSample::GetTimesInt()| rather than keeping another copy of the times vector in the SubSample itself that needs sorting, etc., and won't be used for CPU code)

Anyway, let me know when that's working & I'll do a proper review again (and ask @ast0815 https://github.com/ast0815 to take a look at the time book-keeping)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-617669117, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQZKPPF6CRY4RTFTNHLRN23IZANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

Hi Federico. If you look on github, there are conflicts that need to be resolved before we can merge this PR. So you've got 2 options

  1. Take the merge I did yesterday. Sorry I assumed you used ssh access to github. I see now you're using https. Therefore the remote is different 
    git remote add tom2 https://github.com/tdealtry/TriggerApplication.git
git fetch tom2 feature/CPU_and_GPU
git pull tom2 feature/CPU_and_GPU
git push origin feature/CPU_and_GPU
  2. Do the merge yourself 
    git remote add upstream https://github.com/HKDAQ/TriggerApplication.git
git fetch upstream Application
git pull upstream Application

    Then after resolving the conflicts with git add, git commit, etc.

    git push origin feature/CPU_and_GPU

And just an FYI for future reference: clicking "fork" on github only does something the first time you click it. Clicking it won't update your branches if you've forked previously. Which is why you need to pull manually (I see both https://github.com/federiconova/TriggerApplication/tree/feature/CPU_and_GPU and https://github.com/federiconova/TriggerApplication/tree/Application have as their latest commits (not from you) as from 2 months ago, 20 commits behind)

federiconova commented 4 years ago

Seriously, I don' follow the logic.

Ok, so I did instructrions in (1). Your code is ok on CPU, but does not compile on GPU (make GPU) because of some SubSample problem. Am I to fix it? Will I be able to commit changes into your remote?

On 23/04/2020 11:11, Tom Dealtry wrote:

Hi Federico. If you look on github, there are conflicts that need to be resolved before we can merge this PR. So you've got 2 options

  1. Take the merge I did yesterday. Sorry I assumed you used ssh access to github. I see now you're using https. Therefore the remote is different

|git remote add tom2 https://github.com/tdealtry/TriggerApplication.git git fetch tom2 feature/CPU_and_GPU git pull tom2 feature/CPU_and_GPU git push origin feature/CPU_and_GPU |

  1. Do the merge yourself

|git remote add upstream https://github.com/HKDAQ/TriggerApplication.git git fetch upstream Application git pull upstream Application |

Then after resolving the conflicts with |git add|, |git commit|, etc.

|git push origin feature/CPU_and_GPU |

And just an FYI for future reference: clicking "fork" on github only does something the first time you click it. Clicking it won't update your branches if you've forked previously. Which is why you need to pull manually (I see both https://github.com/federiconova/TriggerApplication/tree/feature/CPU_and_GPU and https://github.com/federiconova/TriggerApplication/tree/Application have as their latest commits (not from you) as from 2 months ago, 20 commits behind)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-618312598, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ6KKHWORDE6I7UBG33ROAH4JANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

Ok great progress! I mentioned this yesterday. SubSample may need changing, but I think I prefer a function like vector<int> SubSample::GetTimesInt() rather than keeping another copy of the times vector in the SubSample itself that needs sorting, splitting, etc., and won't be used by CPU code

If you push to your branch, we don't need to worry about permissions (which will get messy, and go against the pull request workflow we're using)

git push origin feature/CPU_and_GPU
federiconova commented 4 years ago

So...

I shall introduce a function GetTimesInt() in SubSample.*, then do git push?

On 23/04/2020 11:40, Tom Dealtry wrote:

Ok great progress! I mentioned this yesterday. SubSample may need changing, but I think I prefer a function like |vector SubSample::GetTimesInt()| rather than keeping another copy of the times vector in the SubSample itself that needs sorting, splitting, etc., and won't be used by CPU code

If you push to your branch, we don't need to worry about permissions (which will get messy, and go against the pull request workflow we're using)

|git push origin feature/CPU_and_GPU |

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-618326832, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ2VLPJ76AW3O4DZSKDROALIVANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

Sounds good! As long as you git commit first

federiconova commented 4 years ago

Look, you changed something else in temp_m_trigger_search_window_step, and now it doesn't work (segfaults thinking step = 0).

Seriously, I had a code that worked fine yesterday. Now it's broken. Why can't my code that I made a pull request for be considered? I am just chasing after changes that I presume you made...

Federico

On 23/04/2020 12:11, Tom Dealtry wrote:

Sounds good! As long as you |git commit| first

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-618340602, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ55BC3FNTDPJ54X3SLROAO7NANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

Ok, sorry about that. But unfortunately the pull request you made wasn't in a state that was mergable. You can resolve the conflicts with the current version of HKDAQ:Application yourself in the following way First, get rid of my attempt (warning, this will also remove anything you've committed since you pulled my 4 commits, and I presume will also remove anything you've changed but not committed)

git reset --hard e6c85bad92a47f0e785f8b048d4063ae581ad130
git push origin feature/CPU_and_GPU

Then get the latest version of the code

git remote add upstream https://github.com/HKDAQ/TriggerApplication.git
git fetch upstream Application
git pull upstream Application

Then after resolving the conflicts with git add, git commit, etc.

git push origin feature/CPU_and_GPU
federiconova commented 4 years ago

aargh

[ozc82499@hepacc01 TriggerApplication] $ git reset --hard e6c85bad92a47f0e785f8b048d4063ae581ad130 Checking out files: 100% (32/32), done. HEAD is now at e6c85ba update CUDA and WCSimReader; runs on CPU and GPU [ozc82499@hepacc01 TriggerApplication] $ git branch   Application

On 23/04/2020 13:08, Tom Dealtry wrote:

Ok, sorry about that. But unfortunately the pull request you made wasn't in a state that was mergable. You can resolve the conflicts with the current version of HKDAQ:Application yourself in the following way First, get rid of my attempt (warning, this will also remove anything you've committed since you pulled my 4 commits, and I presume will also remove anything you've changed but not committed)

|git reset --hard e6c85bad92a47f0e785f8b048d4063ae581ad130 git push origin feature/CPU_and_GPU |

Then get the latest version of the code

|git remote add upstream https://github.com/HKDAQ/TriggerApplication.git git fetch upstream Application git pull upstream Application |

Then after resolving the conflicts with |git add|, |git commit|, etc.

|git push origin feature/CPU_and_GPU |

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-618364460, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ2S4GO7VPXHPARRW4TROAVSXANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

Does this work? git push origin feature/CPU_and_GPU --force

federiconova commented 4 years ago

yes thanks!

ok I continue following your instructions...

On 23/04/2020 13:13, Tom Dealtry wrote:

Does this work? |git push origin feature/CPU_and_GPU --force|

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-618366707, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ6RCSL4FSBIGENB7BDROAWFRANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

federiconova commented 4 years ago

Ok, I think I did that. Now sure if I fixed all conflicts (how to check that?). But the code I just pushed compiles and runs on both CPU and GPU.

What next?

Thank you...

Federico

On 23/04/2020 13:08, Tom Dealtry wrote:

Ok, sorry about that. But unfortunately the pull request you made wasn't in a state that was mergable. You can resolve the conflicts with the current version of HKDAQ:Application yourself in the following way First, get rid of my attempt (warning, this will also remove anything you've committed since you pulled my 4 commits, and I presume will also remove anything you've changed but not committed)

|git reset --hard e6c85bad92a47f0e785f8b048d4063ae581ad130 git push origin feature/CPU_and_GPU |

Then get the latest version of the code

|git remote add upstream https://github.com/HKDAQ/TriggerApplication.git git fetch upstream Application git pull upstream Application |

Then after resolving the conflicts with |git add|, |git commit|, etc.

|git push origin feature/CPU_and_GPU |

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-618364460, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ2S4GO7VPXHPARRW4TROAVSXANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

Great! Thanks Federico! And sorry again for the confusion with my stuff

How to check? Look at the PR on the github site. It'll say at the bottom if there's any conflicting files

Next step is code review. I'm going to take a look, and ask @ast0815 to look too (mainly with an eye for keeping track of times correctly) Can I please ask that for this bit you reply to comments on the website, rather than via email. The site has a nice feature of being able to comment in line with code. This fails (and it gets very messy!) when replying via email

federiconova commented 4 years ago

Hi, thanks and sorry if I am entirely retarded with anything github related.

I will try to reply inline on the website to all comments. I sense I am going to fail at this...

Federico

On 23/04/2020 14:49, Tom Dealtry wrote:

Great! Thanks Federico! And sorry again for the confusion with my stuff

How to check? Look at the PR on the github site. It'll say at the bottom if there's any conflicting files

Next step is code review. I'm going to take a look, and ask @ast0815 https://github.com/ast0815 to look too (mainly with an eye for keeping track of times correctly) Can I please ask that for this bit you reply to comments on the website, rather than via email. The site has a nice feature of being able to comment in line with code. This fails (and it gets very messy!) when replying via email

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-618404426, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ7PUTCTBY2ZJZKH7E3ROBBN5ANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

federiconova commented 4 years ago

Yes, Lukas should definitely check that the time treatment is correct.

Hey, here is a question. You made a comment to implement CUDA_HOME from the environment. So I will implement it, commit, push, then click on "resolved" in the conversation, is that correct?

On 23/04/2020 14:49, Tom Dealtry wrote:

Great! Thanks Federico! And sorry again for the confusion with my stuff

How to check? Look at the PR on the github site. It'll say at the bottom if there's any conflicting files

Next step is code review. I'm going to take a look, and ask @ast0815 https://github.com/ast0815 to look too (mainly with an eye for keeping track of times correctly) Can I please ask that for this bit you reply to comments on the website, rather than via email. The site has a nice feature of being able to comment in line with code. This fails (and it gets very messy!) when replying via email

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-618404426, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ7PUTCTBY2ZJZKH7E3ROBBN5ANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

Hey, here is a question. You made a comment to implement CUDA_HOME from the environment. So I will implement it, commit, push, then click on "resolved" in the conversation, is that correct?

Yep that should work

And just a warning - ignore all the comments from March (they've all be sorted, or I've commented again) - just check the ones from 5 minutes ago at the bottom of the webpage

federiconova commented 4 years ago

Thanks @federiconova! I think I've just got this comment here, and the comment on this constructor (both in nhits & TestVertices) And the final thing is the FILES folder. Yes, you've convinced me. Can you add an empty .gitignore file in there and commit it? That'll mean the folder will exist for everyone by default

Hi, sorry, which comment on the constructor? The m_time_int? The size changes with every iteration so I don't know how to define it with fixed size outside the loop.

There are still some things that https://github.com/ast0815 should comment on (I am not sure that I am using TimeDelta right and I am not sure where I should add the time offset)

ast0815 commented 4 years ago

The "degradation" you do for comparisons between the GPU and CPU versions of NHits currently only affects whether a trigger is found or not. It does not affect the actual trigger time that is stored by the CPU code. Is that good enough for the comparisons you did/intend to make?

federiconova commented 4 years ago

The "degradation" you do for comparisons between the GPU and CPU versions of NHits currently only affects whether a trigger is found or not. It does not affect the actual trigger time that is stored by the CPU code. Is that good enough for the comparisons you did/intend to make?

Actually you are right. I tried to change it, but I am stuck at how to cast TimeDelta objects to integers or viceversa...

ast0815 commented 4 years ago

To convert a TimeDelta to a double, you have to divide it by a unit:

TimeDelta some_time = TimeDelta(42);
double time_in_nanoseconds = some_time / TimeDelta::ns;
// time_in_nanoseconds == 42.0
double time_in_seconds = some_time / TimeDelta::s;
// time_in_seconds == 42.0e-9

To convert a unitless float, double, or int to a TimeDelta, you can multiply it by the unit:

TimeDelta some_other_time = 42. * TimeDelta::ns;

Or you can use the constructor, which takes a double as argument and interprets it as nanoseconds:

TimeDelta yet_another_time = TimeDelta(42);

The compiler should convert ints to doubles automatically in all these cases.

federiconova commented 4 years ago

thanks, done

tdealtry commented 4 years ago

I think both @ast0815 and myself are happy with this. So it just requires another pull and resolution of conflicts.

I'll just leave a reminder that after WCSim/WCSim#286 kTriggerTestVertices should used, but that will be a separate quick PR

federiconova commented 4 years ago

as in,

git pull AA BB

What's AA and BB? I Have:

[ozc82499@hepacc01 TriggerApplication] $ git remote -v origin    https://github.com/federiconova/TriggerApplication.git (fetch) origin    https://github.com/federiconova/TriggerApplication.git (push) tom    git@github.com:tdealtry/TriggerApplication.git (fetch) tom    git@github.com:tdealtry/TriggerApplication.git (push) tom2    https://github.com/tdealtry/TriggerApplication.git (fetch) tom2    https://github.com/tdealtry/TriggerApplication.git (push) upstream    https://github.com/HKDAQ/TriggerApplication.git (fetch) upstream    https://github.com/HKDAQ/TriggerApplication.git (push) [ozc82499@hepacc01 TriggerApplication] $ git branch   Application

On 06/05/2020 10:00, Tom Dealtry wrote:

I think both @ast0815 https://github.com/ast0815 and myself are happy with this. So it just requires another pull and resolution of conflicts.

I'll just leave a reminder that after WCSim/WCSim#286 https://github.com/WCSim/WCSim/pull/286 |kTriggerTestVertices| should used, but that will be a separate quick PR

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-624526741, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ7MZSFJIKEVADJEMA3RQERKVANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

This PR is against HKDAQ's Application branch, therefore: git pull upstream Aplication

federiconova commented 4 years ago

ok, sorry, as usual I miss the logic

So I did pull upstream and a large number of conflicts shows up. For example in nhits.cpp I have now inherited this line:

triggers->AddTrigger(kTriggerNDigits,                            triggertime - m_trigger_save_window_pre + sample->m_timestamp,                            triggertime + m_trigger_save_window_post + sample->m_timestamp,                            triggertime + sample->m_timestamp,                            std::vector(1, n_digits));

This is just a line I found after git pull. But it doesn't compile:

In file included from UserTools/Factory/../Unity.cpp:5:0,                  from UserTools/Factory/Factory.cpp:1: UserTools/Factory/../nhits/nhits.cpp: In member function ‘void NHits::AlgNDigits(const SubSample*)’: UserTools/Factory/../nhits/nhits.cpp:222:38: error: no matching function for call to ‘TriggerInfo::AddTrigger(ETriggerType, TimeDelta, TimeDelta, TimeDelta, std::vector)’        std::vector(1, n_digits));                                       ^ UserTools/Factory/../nhits/nhits.cpp:222:38: note: candidates are: In file included from include/SubSample.h:9:0,                  from include/DataModel.h:21,                  from include/Tool.h:7,                  from UserTools/Factory/../DummyTool/DummyTool.h:7,                  from UserTools/Factory/../DummyTool/DummyTool.cpp:1,                  from UserTools/Factory/../Unity.cpp:2,                  from UserTools/Factory/Factory.cpp:1: include/TriggerInfo.h:18:8: note: void TriggerInfo::AddTrigger(TriggerType_t, double, double, double, std::vector)    void AddTrigger(TriggerType_t type, double starttime, double endtime, double triggertime, std::vector info);         ^ include/TriggerInfo.h:18:8: note:   no known conversion for argument 2 from ‘TimeDelta’ to ‘double’ include/TriggerInfo.h:21:8: note: void TriggerInfo::AddTrigger(TriggerType_t, TimeDelta, TimeDelta, TimeDelta, TimeDelta, TimeDelta, std::vector)    void AddTrigger(TriggerType_t type, TimeDelta readout_start_time, TimeDelta readout_end_time, TimeDelta mask_start_time, TimeDelta mask_end_time, TimeDelta trigger_time, std::vector info);         ^ include/TriggerInfo.h:21:8: note:   candidate expects 7 arguments, 5 provided

What is going on? There is a very large number of compilation errors like this one.

Federico

On 07/05/2020 11:37, Tom Dealtry wrote:

This PR is against HKDAQ's Application branch, therefore: |git pull upstream Aplication|

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-625173259, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ34WKMTJQ4O444HZGDRQKFN3ANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

In this case, the AddTrigger arguments were changed by me. Sorry, I should have deprecated the old one, rather than just changing it. The new one is shown here https://github.com/HKDAQ/TriggerApplication/blob/Application/DataModel/TriggerInfo.h#L21 And an example of using it in nhits https://github.com/HKDAQ/TriggerApplication/blob/Application/UserTools/nhits/nhits.cpp#L200-L206

federiconova commented 4 years ago

But what I don't get is this. The new code I have inherited by git-pulling now has compilation problems. Here is another one. In nhits.cpp there is a variable

triggers->m_N

which the compiler complains about; I look up in TriggerInfo.h and conclude (but am not quite sure) that it should now be replaced by:

triggers->m_num_triggers

My question is: why is the pulled code not compiling out of the box (a part from the merging conflicts, I understand that part; but the rest?)?

On 07/05/2020 12:54, Tom Dealtry wrote:

In this case, the |AddTrigger| arguments were changed by me. Sorry, I should have deprecated the old one, rather than just changing it. The new one is shown here https://github.com/HKDAQ/TriggerApplication/blob/Application/DataModel/TriggerInfo.h#L21 And an example of using it in nhits https://github.com/HKDAQ/TriggerApplication/blob/Application/UserTools/nhits/nhits.cpp#L200-L206

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-625208785, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQ55Y6TB4IAIATSQUO3RQKOQTANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK

tdealtry commented 4 years ago

Because function arguments have changed (like the first example), or names of data members / functions have changed (like the second example) And yes, triggers->m_num_triggers is the new name for triggers->m_N

federiconova commented 4 years ago

Because function arguments have changed (like the first example), or names of data members / functions have changed (like the second example) And yes, triggers->m_num_triggers is the new name for triggers->m_N

done now?

tdealtry commented 4 years ago

No conflicts and the Travis build passed. Ready for a merge @brichards64 Thanks for all your hard work on this @federiconova!

federiconova commented 4 years ago

I never thought I'd see the end of this...

Thank you for patience!

On 07/05/2020 14:39, Tom Dealtry wrote:

No conflicts and the Travis build passed. Ready for a merge @brichards64 https://github.com/brichards64 Thanks for all your hard work on this @federiconova https://github.com/federiconova!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HKDAQ/TriggerApplication/pull/31#issuecomment-625261793, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJRPQY4QR2GPSCXLNBZVU3RQK2ZXANCNFSM4LF4KPNQ.

--

Federico Nova office: R1 2.89 phone: 01235 44 6265 email: federico.nova@stfc.ac.uk

Particle Physics Department Rutherford Appleton Laboratory Harwell, Didcot, Oxford, OX11 0QX, UK