Comment on revision r289 in aimc

[ai...@googlecode.com]

Comment by r...@google.com:

Score: Positive

General Comment:
Lots of naming and style nits and suggestions.

Some of these comments are probably better left to further discussion, e.g.
those about naming that conflict with the matlab code.

There are other issues similar to the ones I commented on throughout the
library, especially with regard to code commenting. I tried to only
comment once on each issue, instead of drowning the discussion with an
exhaustive list.

Line-by-line comments:

File: /trunk/carfac/agc_coeffs.h (r289)
===============================================================================

Line 1: //
-------------------------------------------------------------------------------
There is a large proliferation of simple headers that define a single
simple struct.

I think the library would look much less daunting (and cluttered) if these
were consolidated a bit, e.g. by grouping all of the AGCCoeffs, AGCState,
and AGCParams struct in a single file called e.g. agc.h. Similary for CAR*
and IHC*.
File: /trunk/carfac/car_state.h (r289)
===============================================================================

Line 29: FloatArray z1_memory;
-------------------------------------------------------------------------------
nit: does suffixing '_memory' to all members really buy anything?

I guess this is trying to be consistent with the matlab naming conventions?
Line 30: FloatArray z2_memory;
-------------------------------------------------------------------------------
These members have very cryptic names. IMHO it would be very useful to add
some more descriptive comments.

Throughout this library.
File: /trunk/carfac/carfac.h (r289)
===============================================================================

Line 61: void Design(const int n_ears, const FPType fs, const CARParams&
car_params,
-------------------------------------------------------------------------------
Shouldn't this work be done by the constructor? (This feels like an
argument I've had several times in the past with Dick, I'll let his choices
win here :)
Line 63: // The 'Run' method processes an entire file with the current
model, using
-------------------------------------------------------------------------------
nit: I think it's more readable to leave a blank line before each method
comment. It's probably okay to bunch together methods that don't have
comments if you want.

I don't insist.
Line 71:
-------------------------------------------------------------------------------
Do we really want this (and other) class(es) to be copyable? I think that
the compiler-generated defaults are likely to work, but unintentional
copies can end up being expensive.

I'd argue that it's a good idea to DISALLOW_COPY_AND_ASSIGN throughout this
library, following
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_Constructors#Copy_Constructors
Line 80: void CloseAGCLoop();
-------------------------------------------------------------------------------
nit: I prefer to leave a blank line between method and member variable
declarations.
Line 81: int n_ears_; // This is the number of ears.
-------------------------------------------------------------------------------
nit: This is a perfect example of a superfluous comment since all it does
is repeat what the code says.
Line 83: int n_ch_; // This is the number of channels in the CARFAC
model.
-------------------------------------------------------------------------------
but these two are okay because it's not obvious what the names mean.

(FWIW, I'd prefer to s/fs/sample_rate/ and s/ch/channels/ throughout the
C++ and Matlab code. Similarly the style guide suggests that the prefix be
changed by s/ n_/ num_/
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=General_Naming_Rules#General_Naming_Rules).
But I don't insist that you fix these minor details now. Consistency in
naming between the C++ and Matlab code is paramount, I think.)
File: /trunk/carfac/carfac_common.h (r289)
===============================================================================

Line 54: using namespace Eigen;
-------------------------------------------------------------------------------
using declarations like this are forbidden by the style guide:

"You may not use a using-directive to make all names from a namespace
available."

In general using declarations are not allowed at the file level inside
header files:

"You may use a using-declaration anywhere in a .cc file, and in functions,
methods or classes in .h files.
// OK in .cc files.
// Must be in a function, method or class in .h files.
using ::foo::bar;"

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namespaces#Namespaces
Line 57: // It's currently set to double for during the unit testing phase
of the
-------------------------------------------------------------------------------
nit: It would make more sense to me for the default to follow the expected
usage of this code, which will likely be float for efficiency.

Is there any way to override a typedef in a common header like this for use
in tests (assuming you really need double precision there)? We'll probably
need a better strategy for this eventually, maybe using templates? (This
came up before, didn't it?)

I don't know if there is anything actionable here right now, except maybe
for switching the default to float if you think it's okay to test at single
precision.
Line 62: typedef Eigen::Array<FPType, Dynamic, 1> FloatArray;
-------------------------------------------------------------------------------
I think we should rename this to ArrayX to be consistent with Eigen
conventions
(http://eigen.tuxfamily.org/dox/TutorialArrayClass.html#TutorialArrayClassTypes)
Line 72: FPType ERBHz(const FPType cf_hz, const FPType erb_break_freq,
-------------------------------------------------------------------------------
This function only appears to be used inside carfac.cc now. Move it there
instead of polluting a common header?
Line 75: // Function CARFACDetect
-------------------------------------------------------------------------------
The rest of this file is going to be useful for SAI code as well as CARFAC
code. I think it should be renamed to common.h.

This function is the lone holdout. Maybe move it to a carfac-specific
shared header, e.g. carfac_common.h or carfac_util.h. I slightly prefer
the latter.
Line 78: FloatArray CARFACDetect (const FloatArray& x);
-------------------------------------------------------------------------------
nit: extra space before the '('
File: /trunk/carfac/carfac_output.h (r289)
===============================================================================

Line 30: // contains a set of two dimensional float arrays (FloatArray2d)
representing
-------------------------------------------------------------------------------
This comment is pretty out of date. There is no more FloatArray2d
(although I'm seriously considering bringing it back as (at least) the
output of SAI::Run which is naturally a 2D array).

In general I think that readability is greatly improved if the code itself
is commented, i.e. describing details of the arrays in a comment above the
definition of the data members (e.g. on line 64) or the accessors, instead
of having a monolithic comment at the very top of the file.

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Function_Comments#Function_Comments
and the surrounding sections.
Line 33: // The 'InitOutput' method is used to initialize the arrays in
each of the
-------------------------------------------------------------------------------
This comment really belongs above the definition InitOutput (which is now
called Init). Similarly for all method- (or member-, as described above)
specific documentation.

Keeping the comments separated from the code as is done here is a recipe
for letting the two get out of sync. Stale comments can be misleading.
Keeping the comments close to the code makes it much more obvious to you
(and any code reviewers) what should change when you're updating code,
decreasing the likelihood that things will go stale.

(Same for keeping comments to a minimum -- IMHO comments should only say
things that aren't immediately obvious from a cursory look at the code.)

You should also get rid of the preamble "The 'InitOutput' method" since the
location of the comment will make it clear what method it refers to, and
saying less will make it less likely to go stale.
Line 42: #include "carfac_common.h"
-------------------------------------------------------------------------------
nit: It is common google style to separate standard library #includes from
those that are project specific with a blank line. See the example in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_and_Order_of_Includes#Names_and_Order_of_Includes
Line 48: const bool store_bm, const bool store_ohc, const bool
store_agc);
-------------------------------------------------------------------------------
nit: funny indentation here.
Line 49: void StoreOutput(const std::vector<Ear>& ears);
-------------------------------------------------------------------------------
Would it make more sense to call this AppendOutput (or even better Append)
instead, since that better reflects what it actually does?
Line 50: // Here we define several acessors for the data members.
-------------------------------------------------------------------------------
nit: This is a perfect example of the kind of comment that tells the reader
nothing. It's quite clear from their inlined definitions that the
following methods are accessors.
Line 51: const std::deque<std::vector<FloatArray>>& nap() { return nap_; }
-------------------------------------------------------------------------------
nit: accessors like this should be labeled const since they cannot mutate
the object. I.e.

const ...& nap() const {...}

From
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of_const#Use_of_const

"Declare methods to be const whenever possible. Accessors should almost
always be const."
Line 64: std::deque<std::vector<FloatArray>> nap_;
-------------------------------------------------------------------------------
I'd be more explicit about the different "dimensions" of these data
structures. It looks like the inner FloatArray is indexed by cochlear
channel (i.e. size n_ch), the vector should have dimension n_ears, and the
outer deque is the number of frames? So a comment like this would be
really useful here:

// CARFAC outputs are stored in nested containers with dimensions n_frames
x n_ears x n_ch.
File: /trunk/carfac/ear.h (r289)
===============================================================================

Line 39: void InitEar(const int n_ch, const FPType fs,
-------------------------------------------------------------------------------
nit: It will almost always be very clear from the code that the object you
are calling InitEar has type Ear. I'd call this method Init.

For more information:
https://code.google.com/p/aimc/source/detail?r=289