[evla-sw-discuss] [Fwd: Re: Code Review]

[Barry Clark]

> 
> i found my notes for this review at the bottom of my backpack this afternoon, so
> am transcribing them for posterity.  mea culpa for not sending them out earlier.
> 
> 	-bryan
> 
> 
> 
> 
> Review Team
> ============
> Author: Barry Clark
> Chairperson: H. Ben Frej (standing in for P Whiteis, who had a conflict)
> Scribe: B Butler (standing in for J Benson, who didn't show up)
> Lector: Dave Harland
> Member: K. Sowinski
> 
> 
> Code of Interest
> ============
> Module:  Subarray.java
> 
> A fairly long (337 lines) module, but much of it is boring code to
> take a message and ship it off to all the Antennas who belong to the
> Subarray.
> 
> Source:
> file:/home/mchost/bclark/NRAO/EVLA/OBSERVE/target/docs/xref/index.html
> and click on Subarray
> 
> Javadoc API:
> file:/home/mchost/bclark/NRAO/EVLA/OBSERVE/target/docs/apidocs/index.html
> 
> 
> Preparation
> ============
> HBF - 1.5h; DH - 1.5h; BGC - 1h; BJB - 0.5h (serendipitously); KS - 0h
> 
> 
> Context
> ============
> . Subarray created at script startup
> . antennas are supported with AntennaPhysical
> . Subarray mostly allows for things to be done over multiple antennas,
>    but also allows for adding and removing antennas from real subarrays
> 
> 
> Notes of Interest
> ============
> . line 55 - method addAntenna - DH points out that this is the place that
>    you should make sure that a new antenna that gets added to a subarray
>    gets initialized correctly - like focus, wrap, etc.  BGC agrees (says
>    it is 'food for thought').

First, note that this is the Antenna data object that is being added.  The
Antenna is already being used by this Array, and things like focus, wrap,
etc are already "properly" set for that Antenna.  This is not the case
for adding a new AntennaPhysical to the Array, which is not being handled
properly either, but that's in another module.

That said, there may need to be something done here.  As currently 
written, it requires at least one source change in both of the donor and 
receiver Subarrays to get the physical antenna pointed at the right source.
I could change it so it requires only a source change in the donor Subarray,
but it needs more thought as to what the proper behavior really is.

> 
> . line 103 - method setLoIfSetup - DH points out that Java 5.0 allows
>    enumerations to be done much more easily.  similar comments apply
>    throughout, where similar looping constructs exist.  BGC agrees, and
>    comments that those loops are 'fossils'.
> 
> . lines 136 & 139 - method setSource - naming the Source as "lo" is clearly
>    a cut-and-paste error.  BGC agrees.

Not an error - merely a nuttiness.  Changed.

> 
> . line 188 - method restoreInterferometerModel - BJB points out that the
>    comments for this method should make clear the fact that this restores
>    the model to CALC explicitly.  BGC agrees.
> 
done

> . line 246 - method registerPointing - BJB points out that you should
>    probably call registerPointing for each.  BGC agrees.
> 
Done.

> . line 283 - method execute - DH queries on units of time.  BGC answers
>    days.  all note that this should be added to comments.

Done.
> 
> . lines 292,293 - method execute - all point out that 1/86400 should
>    be added as a constant rather than written explicitly.
> 
> . lines 294-297 - method execute - DH queries as to why the sequence
>    is precalculateA, calculateA, calculateA, executeA, and why those
>    particular values of time are used (ttime - 0.5*deltaTime, ttime
>    + 0.5*deltaTime, ttime + 1.5*deltaTime)?  BGC explains, but all
>    agree that there needs to be some commenting of this.  BJB points
>    out that it might be better (more understandable) to split it into
>    a calcInitialize, calculate, calculate, calculate sequence, so that
>    it's more clear what is doing what (since precalculate just does an
>    initialize of CALC, then a call to calculate).  BGC seems unconvinced
>    on this latter.
> 
Added comments

> . lines 305, 307 - method execute - BJB points out that these println
>    statements need to go to the logger instead of just being printed.
>    BGC agrees.  also, 'interupted' -> 'interrupted' (spelling error).
> 
Done.
> 
> 
> _______________________________________________
> evla-sw-discuss mailing list
> evla-sw-discuss at listmgr.cv.nrao.edu
> http://listmgr.cv.nrao.edu/mailman/listinfo/evla-sw-discuss
>