Feature/azglam sbnd trackvalidation - Githubissues

SBNSoftware / sbnci

Packages, modules and scripts for the SBN continuous integration system

2 stars 4 forks source link

Feature/azglam sbnd trackvalidation #27

Closed AlaZglam closed 2 years ago

AlaZglam commented 2 years ago

This branch started today for sbnd track validation.

@absolution1 and @etyley, could you please review the physics side of this commit? @henrylay97 and @vitodb, could you please review the CI side of this commit?

many thanks

henrylay97 commented 2 years ago

Then I expect also a CI Validation configuration in lar_ci for this SBND trackvalidation.

Yep once Ed's review is responded to I will include this in the Reco_All workflow

wesketchum commented 2 years ago

@AlaZglam status on this? Do Ed's comments make sense/working on them?

chilge commented 2 years ago

@AlaZglam following up from Wes.

AlaZglam commented 2 years ago

@wesketchum @chilge Ed's comments make sense. I was on holiday, and I have just returned. Sorry, I forgot to do the correction before going on holiday. I'll do the correction ASAP.

chilge commented 2 years ago

Don't worry @AlaZglam ! Going on holiday is important. I hope you are coming back recharged :). I just wanted to make sure this didn't fall through the cracks.

Thanks for jumping on this!

chilge commented 2 years ago

@AlaZglam is this something that be finished today?

AlaZglam commented 2 years ago

@AlaZglam is this something that be finished today? @chilge Sorry for the late response. It will be by the end of today.

chilge commented 2 years ago

@AlaZglam just following up.

chilge commented 2 years ago

@AlaZglam do the above commits address each of @etyley's comments above? can you reply to each question/comment above and mark as resolved if resolved? thanks.

chilge commented 2 years ago

@etyley it appears all issues have been resolved. is this PR good to go?

henrylay97 commented 2 years ago

I haven't looked at the responses to Ed's review but as a side point there seem to be some issues with the merge with develop. You can see in the final commit in particular many elements like this:

<<<<<<< HEAD
find_ups_product( lar_ci )
=======
# commenting this out suppresses CMake warning, "find_ups_product(): 
# REQUIRED UPS product lar_ci has not been set up with WANT_UPS == TRUE!
# find_ups_product( lar_ci ) #commenting this out suppresses CMake warning
>>>>>>> origin

Where no choice of merging has been made.

chilge commented 2 years ago

@AlaZglam can you resolve the outstanding merge conflicts?

chilge commented 2 years ago

this looks good to me. I was able to merge this into my local develop branch and build with no issues. @etyley have your concerns been addressed?

etyley commented 2 years ago

Yeah, my concerns have been addressed

chilge commented 2 years ago

thanks @AlaZglam for the nice addition!