Project

General

Profile

Feature #66

Optics refactor

Added by Rogers, Chris almost 11 years ago. Updated almost 9 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Optics
Target version:
Start date:
26 May 2011
Due date:
01 January 2012
% Done:

100%

Estimated time:
(Total: 0.00 h)
Workflow:
In Development

Description

The optics app is becoming rather unwieldy and clearly needs to be fettled

  • Simulation manager - shared code between Optics and Simulation goes into a "Simulation manager" class (that manages the interface with GEANT4)
  • Optimiser - wrapper for minuit with opportunity to add e.g. Newton Raphson solver
  • Output - should go into some persistency object
  • Optics - should move down into Optics/TransportManager

Files

coverage.tgz (2.45 MB) coverage.tgz Rogers, Chris, 23 January 2012 11:53

Subtasks

Feature #477: CovarianceMatrix.cc/.hh clean upClosedLane, Peter26 May 201101 January 2012

Actions
Feature #476: PhaseSpaceVector clean upClosedLane, Peter26 May 201101 January 2012

Actions

Related issues

Related to MAUS - Feature #624: TransferMap cleanupClosedLane, Peter05 August 201101 January 2012

Actions
#1

Updated by Rogers, Chris almost 11 years ago

  • Category set to Optics
  • Assignee set to Rogers, Chris
#2

Updated by Tunnell, Christopher almost 11 years ago

curious: what interface does optics have with geant4?

#3

Updated by Rogers, Chris almost 11 years ago

At the moment, optics uses tracking to calculate transfer matrix.

So the aim of optics is to get an operator that transports particles from z1 to z2. Basically everyone defines these operators as polynomials in the dynamic variables (in G4MICE case, x,y,time,px,py,energy). One way to calculate the polynomial numerically is to track a whole load of particles and then do a fit to the tracking data. This is how Optics works. There are other ways to do it that I want someone to implement in the future...

So the tracking is provided by GEANT4.

#4

Updated by Rogers, Chris almost 11 years ago

  • Due date set to 01 March 2011
#5

Updated by Rogers, Chris almost 11 years ago

  • Start date changed from 01 October 2010 to 01 February 2011
#6

Updated by Rogers, Chris over 10 years ago

  • Project changed from G4MICE to MAUS
  • Category deleted (Optics)
#7

Updated by Rogers, Chris over 10 years ago

  • Category set to Optics
  • Target version set to Future MAUS release
#8

Updated by Rogers, Chris about 10 years ago

The basic aim of the optics package is to quickly evolve accelerator physics parameters of interest through the MAUS geometry description - i.e. an abritrary set of fields and materials. Accelerator physics sits on top of some fundamental ideas that come from classical mechanics:

  • We can write a Hamiltonian for a particle like H(x,y,time,px,py,energy; z). z is here the independent variable that is used for integration. (x,y,time,px,py,energy) is a phase space vector that marks the position of a particle in phase space.
    • We should use canonical coordinates properly, but most stuff works in kinetic coordinates.
    • Accelerator physics usually uses z (or s for rotating frames, when bending magnets are involved). Possible also to use time, proper time as independent variables, but it's not done much.
  • It is possible to invent some mapping M from position z=zi to z=zf. This maps (x,y,time,px,py,energy) at zi to (x,y,time,px,py,energy) at zf. Typically this is done as a Taylor series about some reference trajectory (so polynomial expansion about a reference).
  • By definition this mapping can transport single particles.
  • It is also possible to transport particle distributions:
    1. We write a particle distribution as a series of beam moments, like <xy> = integral dx dy ( x y ) / integral dx (x) /integral dy (y)
    2. Then one can show that <xf yf> = < M(xf) M(yf) >
  • Then most accelerator physics parameters are function of the second moments <x y>.

So to complete our aim of propagating accelerator physics parameters through an arbitrary set of fields and materials, we need to:

  1. Calculate the Taylor series expansion that makes the transfer map
  2. Propagate sets of moments (esp first and second moments) through the map
    1. Probably useful to also be able to propagate single particles

In MAUS, the accelerator stuff has a few classes associated with it (concept):

  • TransferMap defines the mapping from zi to zf
  • PhaseSpaceVector defines a particle position in phase space
  • CovarianceMatrix defines a set of first and second moments
  • MomentHeap defines a set of arbitrary order moments
  • Material controls what happens to a particle when it goes through some material
  • TransferMapCalculator has some routines for calculating transfer maps
  • TransportManager handles transport through several transfer maps at once
  • Optics handles application level stuff. A lot of things which should properly be delegated to e.g. TransferMapCalculator and TransportManager are owned by Optics.
The following classes are really legacy and need to be eradicated
  • MICEStackingAction (interface to g4)
  • MICETrackingAction (interface to g4)
  • OpticsModel (interface to field maps)
  • Tensor/Tensor3 (superseded by src/Interface/PolynomialVector)
  • AnalysisPlaneBank
#9

Updated by Rogers, Chris about 10 years ago

ps: some reading material is

Dragt, Neri, Rangarajan, General Moment Invariants for linear Hamiltonian Systems, Phys Rev A, Vol 45 Number 4, 1992
Dragt, Numerical Third Order Transfer Map for a Solenoid, NIM A 298, 1990

The Lie algebra stuff - well, we can think about that another time. Main thing is to get the idea of what they are doing i.e. power law expansion round the Hamiltonian to propagate particles, beam moments.

Andy Wolski does some useful lectures for UK Cockroft Institute on this sort of stuff. Worth digging around for.

#10

Updated by Rogers, Chris about 10 years ago

I found that the optics executable didn't work since I changed some of the data structure handling in simulation (returning segmentation fault or something equally nasty). I fixed the problem and it works on my machine. Currently trying to add an integration test layer to the test servers - of which Optics will be one of the tests (checks fields work correctly, for example). Just trying to convince jenkins to build the optics test executable (moved to tests/integration/test_optics so that nosetests can see it)...

#11

Updated by Rogers, Chris almost 10 years ago

  • Assignee changed from Rogers, Chris to Lane, Peter
#12

Updated by Rogers, Chris over 9 years ago

  • Workflow set to In Development

Email from Peter Lane:

Chris,

Revision 660 in branch ~plane1/maus/devel has a successful TransferMap test. Some caveats:

1) The CppErrorHandlerTest has consistently been failing despite occasional reverse merges from the trunk into my dev branch. I briefly looked at it but couldn't make > any sense of it.
2) I had not run cpplint at this point. Problems with later revisions that are broken are because I'm trying to adhere to cpplint warnings (primarily, avoiding "using > namespace" declarations caused a lot of headaches).
3) All the code is still in the legacy directory with some new code in the same files but in the MAUS namespace. It's ugly, but it helped me avoid modifying too much > at once.

Pete

#13

Updated by Rogers, Chris over 9 years ago

So I just built and ran r660

CppErrorHandlerTest - this is a pass on my machine. It makes a whole bunch of error messages, but the test is checking that we can make errors so that's okay. I get

[----------] 4 tests from CppErrorHandlerTest
[ RUN      ] CppErrorHandlerTest.HandleSquealTest
Traceback (most recent call last):
  File "/home/cr67/G4MICE/MAUS/maus_lane/src/common_py/ErrorHandler.py", line 159, in HandleCppException
    raise(CppError(error_message))
ErrorHandler.CppError: a_test at exc::test
[       OK ] CppErrorHandlerTest.HandleSquealTest (9 ms)
[ RUN      ] CppErrorHandlerTest.HandleStdExcTest
Traceback (most recent call last):
  File "/home/cr67/G4MICE/MAUS/maus_lane/src/common_py/ErrorHandler.py", line 159, in HandleCppException
    raise(CppError(error_message))
ErrorHandler.CppError: a_test at exc::test
[       OK ] CppErrorHandlerTest.HandleStdExcTest (0 ms)
[ RUN      ] CppErrorHandlerTest.HandleStdExcNoJsonTest
Traceback (most recent call last):
  File "/home/cr67/G4MICE/MAUS/maus_lane/src/common_py/ErrorHandler.py", line 159, in HandleCppException
    raise(CppError(error_message))
ErrorHandler.CppError: a_test at exc::test
[       OK ] CppErrorHandlerTest.HandleStdExcNoJsonTest (1 ms)
[ RUN      ] CppErrorHandlerTest.HandleSquealNoJsonTest
Traceback (most recent call last):
  File "/home/cr67/G4MICE/MAUS/maus_lane/src/common_py/ErrorHandler.py", line 159, in HandleCppException
    raise(CppError(error_message))
ErrorHandler.CppError: a_test at exc::test
[       OK ] CppErrorHandlerTest.HandleSquealNoJsonTest (0 ms)
[----------] 4 tests from CppErrorHandlerTest (10 ms total)

no red FAIL or ERROR or whatever so it's fine.

I'll try to do the merge... I'll ignore the cpplint errors so long as you plan to fix them when we come out of legacy...

#14

Updated by Rogers, Chris over 9 years ago

Humm looking at it there's quite a few style problems (I have 2500 lines of style errors). Can you spend a day going through and fixing? At that rate maybe quickest to do a script(!)

#15

Updated by Lane, Peter over 9 years ago

I've already fixed them. The problem I'm currently having is that the GetAvgChi2OfDifference and LeastSquaresFitting PolynomialVector unit tests are failing and I haven't had a chance to figure out why. I'm hoping to get back to that this week.

#16

Updated by Rogers, Chris over 9 years ago

Great - yeah, I pulled r660 and tried to merge that one. I would really like to merge this stuff, it looks good. If you could get it to pass all tests that would be great.

Nb: I notice you made some changes in EMRHit and TOFHit. Looked like sensible stuff like adding namespace, did it not compile on your machine or something?

#17

Updated by Lane, Peter over 9 years ago

Could you be more specific about EMRHit and TOFHit. I'm not recalling what you're referring to; and the only trace I see of these names is in MICEEvent:

./legacy/Interface/MICEEvent.hh:19:class EMRHit;
./legacy/Interface/MICEEvent.hh:71: std::vector<EMRHit*> emrHits;

#18

Updated by Rogers, Chris over 9 years ago

It's probably an aside, but for example

=== modified file 'src/legacy/DetModel/EMR/EMRSD.hh'
--- src/legacy/DetModel/EMR/EMRSD.hh    2011-06-02 11:37:46 +0000
+++ src/legacy/DetModel/EMR/EMRSD.hh    2012-01-10 16:43:12 +0000
@@ -16,9 +16,7 @@
 * EMR calorimeter simulation.
 **/

-using namespace MAUS;
-
-class EMRSD : public MAUSSD {
+class EMRSD : public MAUS::MAUSSD {
   public:

       //! Constructor

EMRSD.hh_diff lines 1-15/15 (END) 
=== modified file 'src/legacy/DetModel/EMR/EMRSD.cc'
--- src/legacy/DetModel/EMR/EMRSD.cc    2011-05-28 20:21:39 +0000
+++ src/legacy/DetModel/EMR/EMRSD.cc    2012-01-10 16:43:12 +0000
@@ -49,6 +49,7 @@
 G4bool EMRSD::ProcessHits(G4Step* aStep,G4TouchableHistory* ROhist)
 {
        std::cout << _module->fullName() << std::endl;
+  return false;
        /*
        Json::Value hit;
     G4double length = aStep->GetStepLength();

EMRSD.cc_diff lines 1-12/12 (END) 
#19

Updated by Lane, Peter over 9 years ago

To summarize our short IRC chat about this...

I likely changed this to avoid compile errors. Putting "using" directives in header files is bad because it carries through to any file that includes it.

#20

Updated by Lane, Peter over 9 years ago

From issue #624:

I moved the new vector and matrix classes to common_cpp/Maths. I moved the new CovarianceMatrix, PhaseSpaceVector, and TransferMap classes in common_cpp/Optics. I resolved all of the cpplint errors for these classes except some which aren't valid. I removed MVector and MMatrix and updated all classes that used these to use the new classes.

I also moved PolynomialVector into Maths, but I largely left this unchanged and did not attempt to fix any cpplint errors. I should probably do that at some point, but I'll shift that off onto issue #66. I suppose some supporting files such Interpolator should have been moved as well (more #66 tasks I guess).

So Chris...unless you want me to resolve the cpplint errors for PolynomialVector first, I guess it's ready to merge.

#21

Updated by Rogers, Chris over 9 years ago

Running on my desktop, against r666, I get an exception like "Attempted to assign a matrix of a different size." in MatrixTest.Multiplication (cpp unit tests).

Looks like a problem at line 627:

  Matrix<complex> matrix_c4 = matrix_c0 * matrix_c1;
  EXPECT_TRUE(equal(matrix_c2, matrix_c4));

I want to try to get your test job running on the test server so that I can check I didn't mess up the checkout (and so you can fix this stuff yourself).

#22

Updated by Rogers, Chris over 9 years ago

I got the test server job executing, but it failed with a stack trace. Presumably some problem in PolynomialVector::Chi2SweepingLeastSquaresFit

http://test.mice.rl.ac.uk/job/MAUS_lane/40/console

I think the error

[ RUN      ] PolynomialVectorTest.LeastSquaresFitting

 *** Break *** segmentation violation
===========================================================
There was a crash (#7 0x0505c90d in SigHandler(ESignals) ()).
This is the entire stack trace of all threads:
===========================================================
#0  0x003bd410 in __kernel_vsyscall ()
#1  0x03a41833 in __waitpid_nocancel () from /lib/libc.so.6
#2  0x039e619b in do_system () from /lib/libc.so.6
#3  0x006c9ead in system () from /lib/libpthread.so.0
#4  0x0505839d in TUnixSystem::Exec(char const*) ()
   from /home/jenkins/workspace/MAUS_lane/third_party/build/root_v5.30.03/lib/libCore.so
#5  0x0505fe8d in TUnixSystem::StackTrace() ()
   from /home/jenkins/workspace/MAUS_lane/third_party/build/root_v5.30.03/lib/libCore.so
#6  0x0505c83b in TUnixSystem::DispatchSignals(ESignals) ()
   from /home/jenkins/workspace/MAUS_lane/third_party/build/root_v5.30.03/lib/libCore.so
#7  0x0505c90d in SigHandler(ESignals) ()
   from /home/jenkins/workspace/MAUS_lane/third_party/build/root_v5.30.03/lib/libCore.so
#8  0x05055ad4 in sighandler(int) ()
   from /home/jenkins/workspace/MAUS_lane/third_party/build/root_v5.30.03/lib/libCore.so
#9  <signal handler called>
#10 gsl_matrix_memcpy (dest=0x0, src=0x9679588) at copy_source.c:29
#11 0x0381836e in MAUS::MatrixBase<double, gsl_matrix>::operator=(MAUS::MatrixBase<double, gsl_matrix> const&) ()
   from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#12 0x0381ca85 in MAUS::MatrixBase<double, gsl_matrix>::MatrixBase(MAUS::MatrixBase<double, gsl_matrix> const&) ()
   from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#13 0x0381cbea in MAUS::MatrixBase<double, gsl_matrix>::operator*(MAUS::MatrixBase<double, gsl_matrix> const&) const ()
   from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#14 0x03818b66 in MAUS::Matrix<double>::operator*(MAUS::Matrix<double> const&) const () from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#15 0x0380d845 in MAUS::PolynomialVector::GetAvgChi2OfDifference(std::vector<std::vector<double, std::allocator<double> >, std::allocator<std::vector<double, std::allocator<double> > > >, std::vector<std::vector<double, std::allocator<double> >, std::allocator<std::vector<double, std::allocator<double> > > >) ()
   from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#16 0x03812061 in MAUS::PolynomialVector::Chi2SweepingLeastSquaresFit(VectorMap const&, int, std::vector<MAUS::PolynomialVector::PolynomialCoefficient, std::allocator<MAUS::PolynomialVector::PolynomialCoefficient> >, double, std::vector<double, std::allocator<double> >&, double, int) ()
   from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#17 0x08087797 in PolynomialVectorTest_LeastSquaresFitting_Test::TestBody() ()
#18 0x0456069e in testing::Test::Run (this=0x966d1a8) at ./src/gtest.cc:2095
#19 0x04561a28 in testing::internal::TestInfoImpl::Run (this=0x8bf80b8)
    at ./src/gtest.cc:2314
#20 0x04561acb in testing::TestCase::Run (this=0x8bf7990)
    at ./src/gtest.cc:2420
#21 0x045689a3 in testing::internal::UnitTestImpl::RunAllTests (
    this=0x8beb500) at ./src/gtest.cc:4024
#22 0x04568bb0 in testing::UnitTest::Run (this=0x457ce80)
    at ./src/gtest.cc:3687
#23 0x081342b7 in main ()
===========================================================

The lines below might hint at the cause of the crash.
If they do not help you then please submit a bug report at
http://root.cern.ch/bugs. Please post the ENTIRE stack trace
from above as an attachment in addition to anything else
that might help us fixing this issue.
===========================================================
#10 gsl_matrix_memcpy (dest=0x0, src=0x9679588) at copy_source.c:29
#11 0x0381836e in MAUS::MatrixBase<double, gsl_matrix>::operator=(MAUS::MatrixBase<double, gsl_matrix> const&) ()
   from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#12 0x0381ca85 in MAUS::MatrixBase<double, gsl_matrix>::MatrixBase(MAUS::MatrixBase<double, gsl_matrix> const&) ()
   from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#13 0x0381cbea in MAUS::MatrixBase<double, gsl_matrix>::operator*(MAUS::MatrixBase<double, gsl_matrix> const&) const ()
   from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#14 0x03818b66 in MAUS::Matrix<double>::operator*(MAUS::Matrix<double> const&) const () from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#15 0x0380d845 in MAUS::PolynomialVector::GetAvgChi2OfDifference(std::vector<std::vector<double, std::allocator<double> >, std::allocator<std::vector<double, std::allocator<double> > > >, std::vector<std::vector<double, std::allocator<double> >, std::allocator<std::vector<double, std::allocator<double> > > >) ()
   from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#16 0x03812061 in MAUS::PolynomialVector::Chi2SweepingLeastSquaresFit(VectorMap const&, int, std::vector<MAUS::PolynomialVector::PolynomialCoefficient, std::allocator<MAUS::PolynomialVector::PolynomialCoefficient> >, double, std::vector<double, std::allocator<double> >&, double, int) ()
   from /home/jenkins/workspace/MAUS_lane/build/libMausCpp.so
#17 0x08087797 in PolynomialVectorTest_LeastSquaresFitting_Test::TestBody() ()
#18 0x0456069e in testing::Test::Run (this=0x966d1a8) at ./src/gtest.cc:2095
#19 0x04561a28 in testing::internal::TestInfoImpl::Run (this=0x8bf80b8)
    at ./src/gtest.cc:2314
#20 0x04561acb in testing::TestCase::Run (this=0x8bf7990)
    at ./src/gtest.cc:2420
#21 0x045689a3 in testing::internal::UnitTestImpl::RunAllTests (
    this=0x8beb500) at ./src/gtest.cc:4024
#22 0x04568bb0 in testing::UnitTest::Run (this=0x457ce80)
    at ./src/gtest.cc:3687
#23 0x081342b7 in main ()
===========================================================

FAIL

Interestingly, no problem in Matrix multiplication

[ RUN      ] MatrixTest.Multiplication
[       OK ] MatrixTest.Multiplication (0 ms)

so maybe I screwed something up.

#23

Updated by Lane, Peter over 9 years ago

I should have done a reverse merge. Doing that now. We'll see what Jenkins says...

#24

Updated by Rogers, Chris over 9 years ago

Still a segmentation fault. Nb: I find valgrind is a useful tool for catching this stuff. Takes a bit of getting used to (makes a whole load of false positives in python for example). But once you get used to it, it can usually find the memory problem pretty quick. Also try recompiling MAUS with the debug flag set (set as an environment variable in env.sh like maus_debug or something). Makes line numbers available to the OS/valgrind. If you like debuggers (I find std::cout much more useful) there is also gdb.

#25

Updated by Lane, Peter over 9 years ago

I did a reverse merge over the weekend and I can't reproduce this seg fault on my machine. Jenkins says something failed, but I can't find anything useful in the logs that were generated. The install_log_std file simply ends with

[ RUN ] HermitianMatrixTest.Eigen
FAIL! See logs.

I can't find anything useful in install_log_err. I'll try to find a Linux box to build on. Short of that I'm not sure what else to do at this point.

#26

Updated by Rogers, Chris over 9 years ago

There are a few lines like

*** glibc detected *** /home/jenkins/workspace/MAUS_lane/build/test_cpp_unit: double free or corruption (fasttop): 0x092103e0 ***
======= Backtrace: =========
/lib/libc.so.6[0x30aa6c5]
/lib/libc.so.6(cfree+0x59)[0x30aab09]
/home/jenkins/workspace/MAUS_lane/third_party/install/lib/libgsl.so.0(gsl_vector_complex_free+0x2d)[0xee192d]
/home/jenkins/workspace/MAUS_lane/build/libMausCpp.so(_ZN4MAUS10VectorBaseI11gsl_complex18gsl_vector_complexE13delete_vectorEv+0x28)[0x3446c38]
/home/jenkins/workspace/MAUS_lane/build/libMausCpp.so(_ZN4MAUS10VectorBaseI11gsl_complex18gsl_vector_complexED2Ev+0x1d)[0x34490bd]
/home/jenkins/workspace/MAUS_lane/build/test_cpp_unit[0x80b7250]
/home/jenkins/workspace/MAUS_lane/third_party/install/lib/libgtest.so.0(_ZN7testing4Test3RunEv+0x8e)[0x7f3069e]
/home/jenkins/workspace/MAUS_lane/third_party/install/lib/libgtest.so.0(_ZN7testing8internal12TestInfoImpl3RunEv+0x108)[0x7f31a28]
/home/jenkins/workspace/MAUS_lane/third_party/install/lib/libgtest.so.0(_ZN7testing8TestCase3RunEv+0x9b)[0x7f31acb]
/home/jenkins/workspace/MAUS_lane/third_party/install/lib/libgtest.so.0(_ZN7testing8internal12UnitTestImpl11RunAllTestsEv+0x2c3)[0x7f389a3]
/home/jenkins/workspace/MAUS_lane/third_party/install/lib/libgtest.so.0(_ZN7testing8UnitTest3RunEv+0x20)[0x7f38bb0]
/home/jenkins/workspace/MAUS_lane/build/test_cpp_unit(main+0x167)[0x81342c7]
/lib/libc.so.6(__libc_start_main+0xdc)[0x3056e9c]
/home/jenkins/workspace/MAUS_lane/build/test_cpp_unit[0x8058601]

followed by another 100 odd lines of "Memory map"

http://test.mice.rl.ac.uk/job/MAUS_lane/ws/install_log_err/*view*/

But yes, it is somewhat obscured - annoyingly, python tests stuff seems to go to std::err by default. I think need to merge the install_log_std and install_log_err because the distinction is just confusing.

#27

Updated by Rogers, Chris over 9 years ago

Nb: I don't know whether you are familiar with all the memory issues that turn up in C programming like buffer overflows etc... they tend to produce subtle/inconsistent bugs just like this. Depending on where the buffer was malloc'd, an overflow can overflow into arbitrary code so it can have arbitrary effect - often it overflows into unassigned memory and doesn't do anything.

#28

Updated by Lane, Peter over 9 years ago

Ah, thanks! I had seen that section but missed the munged type names. Yes, I'm familiar with pointer issues, though I'll admit I'm a bit rusty (or perhaps "spoiled" is more accurate). I coded in Java and Perl for the majority of my developer years. I had forgotten about Valgrind. I'm installing that as I type. And yes, it appears that SConstruct looks for maus_debug to set the -g option. Thanks for the info.

#29

Updated by Rogers, Chris over 9 years ago

Looks like it passed tests except cpplint - don't mind fixing some stuff, but new code I think is your responsibility (HermitianMatrix, SymmetricMatrix, ... ...)

http://test.mice.rl.ac.uk/job/MAUS_lane/ws/tmp/cpplint.out/*view*/

#30

Updated by Lane, Peter over 9 years ago

No problem. I thought I had fixed all the cpplint violations. I'll get on it now.

#31

Updated by Lane, Peter over 9 years ago

I just pushed another set of changes that fixes most of the cpplint errors. I have not reformatted PolynomialVector.?? yet. If by "don't mind fixing some stuff" you mean you're willing to fix the errors in those source files, I won't complain. Otherwise I'll have to tackle it some other day.

There were a number of errors that I disagree with...

src/common_cpp/Maths/Complex.hh:120: Is this a non-const reference? If so, make const or use a pointer. [runtime/references] [2]
src/common_cpp/Maths/Complex.hh:122: Is this a non-const reference? If so, make const or use a pointer. [runtime/references] [2]
src/common_cpp/Maths/Complex.hh:124: Is this a non-const reference? If so, make const or use a pointer. [runtime/references] [2]
src/common_cpp/Maths/Complex.hh:126: Is this a non-const reference? If so, make const or use a pointer. [runtime/references] [2]

-- Why do I have to have const references? I don't want to use a pointer!

src/common_cpp/Maths/HermitianMatrix.hh:180: Single-argument constructors should be marked explicit. [runtime/explicit] [5]
src/common_cpp/Maths/Matrix.hh:427: Single-argument constructors should be marked explicit. [runtime/explicit] [5]
src/common_cpp/Maths/Matrix.hh:465: Single-argument constructors should be marked explicit. [runtime/explicit] [5]
src/common_cpp/Maths/Vector.hh:231: Single-argument constructors should be marked explicit. [runtime/explicit] [5]
src/common_cpp/Maths/Vector.hh:263: Single-argument constructors should be marked explicit. [runtime/explicit] [5]
src/common_cpp/Optics/PhaseSpaceVector.hh:46: Single-argument constructors should be marked explicit. [runtime/explicit] [5]

-- Copy constructors are an exception.

tests/cpp_unit/Maths/PolynomialVectorTest.cc:215: Extra space before ( in function call [whitespace/parens] [4]

-- "catch" is not a function but a keyword just like "if" and "for".

#32

Updated by Rogers, Chris over 9 years ago

You can make exceptions to style issues you don't like in tests/style/cpplint_exceptions.py. Just give a reason. The main point is to make people think about the way they write code, if they've thought about it and decided to do it this way or that way, I'm happy.

Lane, Peter wrote:

I just pushed another set of changes that fixes most of the cpplint errors. I have not reformatted PolynomialVector.?? yet. If by "don't mind fixing some stuff"
you mean you're willing to fix the errors in those source files, I won't complain. Otherwise I'll have to tackle it some other day.

That's fine, I'll do it as part of the merge.

There were a number of errors that I disagree with...

src/common_cpp/Maths/Complex.hh:120: Is this a non-const reference? If so, make const or use a pointer. [runtime/references] [2]
src/common_cpp/Maths/Complex.hh:122: Is this a non-const reference? If so, make const or use a pointer. [runtime/references] [2]
src/common_cpp/Maths/Complex.hh:124: Is this a non-const reference? If so, make const or use a pointer. [runtime/references] [2]
src/common_cpp/Maths/Complex.hh:126: Is this a non-const reference? If so, make const or use a pointer. [runtime/references] [2]

-- Why do I have to have const references? I don't want to use a pointer!

The argument here is that a reference is like a pointer, only hidden. So for example, you can make a reference to NULL, or to unassigned memory (I guess it is a bit harder to do than with pointers), and get the usual errors. So why not just use a pointer, at least then it is explicit that you are using a pointer...

src/common_cpp/Maths/HermitianMatrix.hh:180: Single-argument constructors should be marked explicit. [runtime/explicit] [5]
src/common_cpp/Maths/Matrix.hh:427: Single-argument constructors should be marked explicit. [runtime/explicit] [5]
src/common_cpp/Maths/Matrix.hh:465: Single-argument constructors should be marked explicit. [runtime/explicit] [5]
src/common_cpp/Maths/Vector.hh:231: Single-argument constructors should be marked explicit. [runtime/explicit] [5]
src/common_cpp/Maths/Vector.hh:263: Single-argument constructors should be marked explicit. [runtime/explicit] [5]
src/common_cpp/Optics/PhaseSpaceVector.hh:46: Single-argument constructors should be marked explicit. [runtime/explicit] [5]

-- Copy constructors are an exception.

Fine. I think the style guide doesn't really like operator overloading (including copy constructors I guess). I'm not so bothered, so fine.

tests/cpp_unit/Maths/PolynomialVectorTest.cc:215: Extra space before ( in function call [whitespace/parens] [4]

-- "catch" is not a function but a keyword just like "if" and "for".

Good point. We changed the style guide to use exceptions, probably this never made it into cpplint.py

#33

Updated by Rogers, Chris over 9 years ago

ps: I think you forgot to do the commit... I can't see any changes to your branch

#34

Updated by Rogers, Chris over 9 years ago

pps: does this close 477, 476, 624?

#35

Updated by Lane, Peter over 9 years ago

My latest push (revision 671) should be everything except the PolynomialVector source file cpplint errors.

Sure, I'll close out 477, 476, and 624. Any stuff that I pulled out of those classes that needs to go elsewhere can be done as part of this issue.

#36

Updated by Rogers, Chris over 9 years ago

I fixed the phase space vector cpplint errors. However, I didn't want to do the merge because I don't think the test coverage is high enough... I attach the lcov report as coverage.tgz. In general I look at line coverage - I want to keep it above 80% or so. Generate coverage report by installing lcov and doing export maus_lcov=1 before build/test.

My edits are in branch

bzr+ssh://bazaar.launchpad.net/~chris-rogers/maus/lane-merge/

#38

Updated by Rogers, Chris almost 9 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

Closed I think

#39

Updated by Rogers, Chris almost 9 years ago

  • Target version changed from Future MAUS release to MAUS-v0.4.1

Also available in: Atom PDF