Project

General

Profile

Support #595

Code review - DetModel/SciFi/

Added by Santos, Edward almost 11 years ago. Updated almost 10 years ago.

Status:
Rejected
Priority:
High
Category:
Tracker
Target version:
Start date:
22 July 2011
Due date:
05 August 2011
% Done:

0%

Estimated time:
Workflow:

Description

For all code in src/legacy/DetModel/SciFi/ in my branch.

This closes bug #335 while adds documentation.


Files

DoubletFiberParam.cc (3.35 KB) DoubletFiberParam.cc Heidt, Christopher, 13 August 2011 02:59
SciFiSD.cc (5.93 KB) SciFiSD.cc Heidt, Christopher, 13 August 2011 02:59
#1

Updated by Rogers, Chris almost 11 years ago

  • Due date set to 05 August 2011
  • Category set to Tracker
  • Assignee set to Carlisle, Timothy
  • Target version set to Future MAUS release

Assigned to Tim. Note the code review guidelines at:

http://micewww.pp.rl.ac.uk/projects/maus/wiki/Code_review

Get the code with command like:

bzr branch lp:~e-santos10/maus/devel

#2

Updated by Tunnell, Christopher almost 11 years ago

This code needs to be in 'common_cpp' not legacy. You should be able to just move it.

New code is forbidden within legacy.

#4

Updated by Santos, Edward almost 11 years ago

  • Subject changed from Code review request to Code review - DetModel/SciFi/
#5

Updated by Carlisle, Timothy almost 11 years ago

  • Assignee changed from Carlisle, Timothy to Santos, Edward

SciFiPlane.cc

Just a few points:

1) cpplint has flagged ~ 50 spacing errors etc

2) Header file comments are succinct and helpful. You could explain what the logical volume / logicCore /logicDoublet stuff means though, for completeness...as in explicitly differentiate it from physical vol..

3) I assume point 2 of this thread still applies..?

#6

Updated by Santos, Edward almost 11 years ago

Will introduce the required changes in the next submission. Thank you!

#7

Updated by Carlisle, Timothy almost 11 years ago

DoubletFiberParam.cc

1) cpplint: 18 errors

2) This code is basically all geometry e.g.

G4double spacing = sqrt(fiberDiameter*fiberDiameter*
(1-fiberPitch*fiberPitch/4));

the finer details of which are beyond me...are all these formulae tested/verified somehow?

SciFiSD.cc

1) cpplint: 62 errors, spacing again!

2) I think more explanation is necessary to describe what ProcessHits does. Are you reading in hit information/channel for each G4step through the SciFiPlanes?

Users may have little understanding of how these executables fit into the bigger picture of the Tracker Code as a whole, so you could try to put it in context - or include a link showing the overall structure/design of the Tracker software as a whole.

3) If I understand correctly, old_chanNo is only required for comparison with chanNo, which are both output to debug.txt. So is legacy_chanNo() a fully-fledged test now or genuine legacy code to be phased out [and replaced?]?

#8

Updated by Santos, Edward almost 11 years ago

The unit tests are being improved. Parameters like the one you are showing are compared with the values presented/derived from MICE note 135:

http://mice.iit.edu/micenotes/public/pdf/MICE0135/MICE0135.pdf

ES

#9

Updated by Tunnell, Christopher almost 11 years ago

Are you wanting to merge with the trunk? Or are you just wanting input?

This relates to issues:

512 unit tests
507 refactor
506 code review
505 style guide and comments
504 (parent) cleanup scifiSD

Is this all of those issues? I also don't see SciFiSD in your branch anywhere....

http://bazaar.launchpad.net/~e-santos10/maus/devel/files/head:/src

Am I missing something?

#10

Updated by Santos, Edward almost 11 years ago

Sorry, I moved it like you said and forgot to "add" the new folder to bzr. Will do it now.

I'll have to see which issues are related with this, but it's a bunch of them. I'll look tomorrow.

Tunnell, Christopher wrote:

Are you wanting to merge with the trunk? Or are you just wanting input?

This relates to issues:

512 unit tests
507 refactor
506 code review
505 style guide and comments
504 (parent) cleanup scifiSD

Is this all of those issues? I also don't see SciFiSD in your branch anywhere....

http://bazaar.launchpad.net/~e-santos10/maus/devel/files/head:/src

Am I missing something?

#11

Updated by Santos, Edward almost 11 years ago

Chris, this relates to issues

335 - tracker geometry issue when finding fibre number (affects physics resolutions)
504 - Clean up SciFiSD
505 - SciFiSD style guide and comments
506 - SciFiSD code review
507 - SciFiSD refactor
509 - Tracker Geometry
512 - SciFiSD unit tests !!!!!!!!!!!WHICH WILL REMAIN OPEN!!!!!!!!!!!!!!!!!!!!!!!
595 - Code review - DetModel/SciFi/

Tunnell, Christopher wrote:

Are you wanting to merge with the trunk? Or are you just wanting input?

This relates to issues:

512 unit tests
507 refactor
506 code review
505 style guide and comments
504 (parent) cleanup scifiSD

Is this all of those issues? I also don't see SciFiSD in your branch anywhere....

http://bazaar.launchpad.net/~e-santos10/maus/devel/files/head:/src

Am I missing something?

#12

Updated by Tunnell, Christopher almost 11 years ago

So it's without tests? Is that what you mean when saying that that issue stays open?

We can say the code looks good but it can't go into the trunk without tests. It would be nice to finish at least some of the tracker stuff.

#13

Updated by Tunnell, Christopher almost 11 years ago

Can you commit on a date that the unit tests will be done? It really should be done this month for the SciFiSD stuff. Including time to expect a week of corrections.

#14

Updated by Tunnell, Christopher almost 11 years ago

I can't compile. Where'd the files at src/input/InputCppData/ go?

Can you do:

bzr branch ~e-santos10/maus/devel maus-test
cd maus-test
./tests/integration/install_then_build_then_test.bash

and make sure it works? You should also do:

bzr merge lp:maus

to sync with the trunk first. Then make sure your branch works here:

http://test.mice.rl.ac.uk/job/MAUS_santos/

which will get rebuilt every time you commit (within 24 hours)

#15

Updated by Tunnell, Christopher almost 11 years ago

  • Status changed from Open to In Progress
  • Priority changed from Normal to High
  • % Done changed from 0 to 50
#16

Updated by Santos, Edward almost 11 years ago

scons -c; scons

doesn't solve it either - tried before.

#17

Updated by Tunnell, Christopher almost 11 years ago

The trunk builds from scratch on a wide range of machines so something must have changed. Can you see what's different versus the trunk?

#18

Updated by Santos, Edward almost 11 years ago

I can't see what's wrong. I think I'm going to pull a new installation from the trunk and take the files I changed there.

#19

Updated by Tunnell, Christopher almost 11 years ago

If you create a launchpad branch of just the scifiSD stuff then that greatly simplifies including it into the trunk. Smaller but frequent chunks of code being submitted makes everybody's life easier.

#20

Updated by Heidt, Christopher almost 11 years ago

I looked over these and made some comments where I could. I had some questions about DoubletFiberParam.cc which I added into the comments. Also, I looked over SciFiPlane.cc but couldn't find anything to comment on.

#21

Updated by Tunnell, Christopher almost 11 years ago

With Chris Heidt's comments added in (and verified), is this tracker code ready to go into the trunk? I know you just have it put down as 'code review' but is this code finished? It would be nice to cross things off the list.

#22

Updated by Rogers, Chris almost 10 years ago

  • Status changed from In Progress to Rejected
  • % Done changed from 50 to 0

Also available in: Atom PDF