DM-15105: Style guide fixes #138

timj · 2018-07-13T20:05:59Z

Enable flake8 and travis

timj · 2018-07-13T20:10:14Z

@PaulPrice this checking has uncovered a seeming bug:

./python/lsst/obs/subaru/ccdTesting.py:2844:92: F821 undefined name 'median'
./python/lsst/obs/subaru/ccdTesting.py:2847:14: F821 undefined name 'medianFilterImage'

Looking at arcturus.py it seems there is an undisclosed dependency on lsst.analysis and that is where medianFilterImage should be coming from. I don't see setupRequired(analysis) in the table file. median does not seem to be defined anywhere so I'm not sure how you want me to fix this.

timj · 2018-07-13T20:14:36Z

python/lsst/obs/subaru/ccdTesting.py

@@ -1002,7 +986,7 @@ def grads(exp):
    plt.clf()
    for a in ccd:
        ima = im[a.getRawDataBBox()].getArray()
-        I = np.mean(ima, 1)/np.median(ima)
+        I = np.mean(ima, 1)/np.median(ima)  # noqa E741


@PaulPrice not sure if you would rather rename this variable to fix the warning or keep the noqa.

timj · 2018-07-13T20:17:29Z

And matplotlib is imported but not declared in the table file, and import lsst.afw.display.ds9 as ds9 is being used.

timj · 2018-07-13T20:33:43Z

bin.src/showPsfs.py

-                   showNdataFwhm=args.showNdataFwhm, showNdataEll=args.showNdataEll,
-                   minNdata=args.minNdata, maxNdata=args.maxNdata,
-                   verbose=args.verbose)
+        plot = main(dataDir, visit, title, args.outputTxtFile,


This one was causing trouble because plt was clashing with the plt that was imported from matplotlib.

PaulPrice · 2018-07-13T21:05:41Z

Let's jettison ccdTesting.py. OK, @RobertLuptonTheGood ?

RobertLuptonTheGood · 2018-07-14T13:33:57Z

OK. But there's potentially useful stuff in there, so I'd like to see any files removed from obs_subaru put onto a branch (called something like HSCcommissioningTools) before deleting them

timj · 2018-07-16T15:26:15Z

@RobertLuptonTheGood ccdTesting.py is broken. See my comment above. If you want to suggest a fix for the medians I'm happy to integrate it. I can comment out the entire function if you prefer? Putting it on a non-standard branch seems like a bad plan to me.

timj · 2018-07-16T18:21:15Z

I've discussed this on Slack and the plan is to remove the file and be explicit in the commit message. I'm having trouble pushing this change to the branch at the moment.

timj · 2018-07-16T18:26:24Z

@PaulPrice I've made the changes, and added a few more cleanups? Can you take a look or should I ask @r-owen ?

r-owen

A very nice update. I found a few minor issues and have a few questions for things that are probably right but new to me.

r-owen · 2018-07-16T21:41:55Z

bin.src/genDefectRegistry.py

@@ -67,11 +65,12 @@ def __init__(self, path, version, validStart, validEnd=None):
 # Defects files for a CCD are valid from the date they are registered until the next date.
 # This means that any defects file should carry ALL the defects that are present at that time.
 for ccd, rowList in rowsPerCcd.items():
-    rowList.sort(key=lambda row: row.validStart) # ISO-8601 will sort just fine without conversion from str
+    # ISO-8601 will sort just fine without conversion from srt


type: "srt" -> "str"

I have no idea how I transposed those letters.

r-owen · 2018-07-16T21:43:32Z

bin.src/makeGeom.py

@@ -236,7 +234,7 @@ def printElectronics(buff, ccdId, ccdname, xidx, yidx, infoA, infoB):

    # 0th layer is pure metadata
    for ccd in range(1, len(ptr)):
-        #for ccd in [1, 28]:
+        # for ccd in [1, 28]:


Consider deleting commented-out code. This looks even safer to remove than the bit earlier.

r-owen · 2018-07-16T21:43:39Z

bin.src/makeGeom.py

-    #buff.write('        name: "R:0,0 S:%d,%d CFHTid:%d %s" \n' % (xidx, yidx, ccdId, ccdname))
-    #buff.write('        serial: -1 \n')
+    # buff.write('        name: "R:0,0 S:%d,%d CFHTid:%d %s" \n' % (xidx, yidx, ccdId, ccdname))
+    # buff.write('        serial: -1 \n')


Consider deleting commented-out code. I can see why you kept it -- better safe than sorry, especially as it's not your package. @PaulPrice what do you think?

This didn't look like code I wanted to mess with so I fixed the warning only.

I think this file is left over from the cameraGeom transition half a lifetime ago, and it looks like it contains CFHT-specific stuff (?!). Suggest we simply dump it all.

r-owen · 2018-07-16T21:51:10Z

setup.cfg

+    config/*.py ALL
+    suprimecam/* ALL
+    hsc/* ALL
+    bin/* ALL


Is the ALL necessary? I have not seen it used before.
Also, why ignore everything in bin? I would expect all the python code to be from bin.src and to pass.

Testing bin/ and bin.src/ doesn't add anything so I thought it was quicker to ignore them. The ALL might not be necessary, I didn't think it was until last week when a package I was working on with @SimonKrughoff required it. I'll take another look.

I have experimented. See also https://pypi.org/project/pytest-flake8/

Specifying a file in the flake8-ignore section without any parameter does nothing. You need the ALL to prevent pytest from looking at the file at all. flake8 does respect the exclude line in the section above though. So without the ALL pytest sends the file to flake8 and flake8 returns a PASS because it was told to excliude the file. If you specify ALL then pytest never sends the file to flake8 so it doesn't even look like a test. ALL is therefore the correct approach.

r-owen · 2018-07-16T21:52:24Z

ups/obs_subaru.build

@@ -1,2 +0,0 @@
-@LSST BUILD@ &&
-build_lsst @PRODUCT@ @VERSION@ @REPOVERSION@


Why is this file going away?

There was a conversation on Slack last week where we all came to the conclusion that nothing uses these files and they are unused cruft in the system that confuse people.

That is correct.

r-owen · 2018-07-16T21:53:27Z

python/lsst/obs/subaru/strayLight/waveletCompression.py

@@ -14,24 +14,22 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.

-#----------------------------------------------------------------------------
+# ----------------------------------------------------------------------------


I know this is picky and feel free to ignore it, but consider getting rid of the line?

Yes. Sorry. I should have removed it.

timj · 2018-07-16T22:37:11Z

I have made the requested changes. I'll wait until tomorrow before merging in case @PaulPrice has a comment.

laurenam · 2018-07-16T22:42:15Z

Just FYI, @PaulPrice is away through Wednesday.

PaulPrice

Looks good, thanks!

PaulPrice · 2018-07-19T15:29:25Z

bin.src/makeGeom.py

-    #buff.write('        name: "R:0,0 S:%d,%d CFHTid:%d %s" \n' % (xidx, yidx, ccdId, ccdname))
-    #buff.write('        serial: -1 \n')
+    # buff.write('        name: "R:0,0 S:%d,%d CFHTid:%d %s" \n' % (xidx, yidx, ccdId, ccdname))
+    # buff.write('        serial: -1 \n')


I think this file is left over from the cameraGeom transition half a lifetime ago, and it looks like it contains CFHT-specific stuff (?!). Suggest we simply dump it all.

PaulPrice · 2018-07-19T15:30:37Z

bin.src/refileFocus.py

@@ -12,8 +12,6 @@
    108		110


I don't know what this script does. Suggest dumping it.

PaulPrice · 2018-07-19T15:37:43Z

ups/obs_subaru.build

@@ -1,2 +0,0 @@
-@LSST BUILD@ &&
-build_lsst @PRODUCT@ @VERSION@ @REPOVERSION@


That is correct.

timj requested a review from PaulPrice July 13, 2018 20:05

timj commented Jul 13, 2018

View reviewed changes

timj force-pushed the tickets/DM-15105 branch from 2d976ee to be4b091 Compare July 16, 2018 18:21

timj requested a review from r-owen July 16, 2018 21:36

r-owen approved these changes Jul 16, 2018

View reviewed changes

timj force-pushed the tickets/DM-15105 branch from 8cbbb7e to a585b1e Compare July 16, 2018 22:36

timj force-pushed the tickets/DM-15105 branch from a585b1e to f7d29e7 Compare July 17, 2018 23:02

PaulPrice approved these changes Jul 19, 2018

View reviewed changes

timj added 11 commits July 19, 2018 09:56

Enable flake8 configuration

e762743

Use automatic file detection

5775e80

Style fixes for bin files

ce56d68

Style fixes

a6928df

Enable travis flake8

80c8fe1

Fix median bugs in ccdTesting.py

15e0ccf

Remove python2 compatibility

8f033ba

Remove the .build file that is no longer used

f7cc7c8

Remove unused ccdTesting.py file

aa6fc77

Turn comment into module docstring

060968e

Remove commented out code

0953bf4

timj force-pushed the tickets/DM-15105 branch from f7d29e7 to 0953bf4 Compare July 19, 2018 17:04

timj merged commit 0953bf4 into master Jul 19, 2018

ktlim deleted the tickets/DM-15105 branch August 25, 2018 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-15105: Style guide fixes #138

DM-15105: Style guide fixes #138

timj commented Jul 13, 2018

timj commented Jul 13, 2018

timj Jul 13, 2018

timj commented Jul 13, 2018

timj Jul 13, 2018

PaulPrice commented Jul 13, 2018

RobertLuptonTheGood commented Jul 14, 2018 via email

timj commented Jul 16, 2018

timj commented Jul 16, 2018

timj commented Jul 16, 2018

r-owen left a comment

r-owen Jul 16, 2018

timj Jul 16, 2018

r-owen Jul 16, 2018

r-owen Jul 16, 2018

timj Jul 16, 2018

PaulPrice Jul 19, 2018

r-owen Jul 16, 2018

timj Jul 16, 2018

timj Jul 16, 2018

r-owen Jul 16, 2018

timj Jul 16, 2018

PaulPrice Jul 19, 2018

r-owen Jul 16, 2018

timj Jul 16, 2018

timj commented Jul 16, 2018

laurenam commented Jul 16, 2018

PaulPrice left a comment

PaulPrice Jul 19, 2018

PaulPrice Jul 19, 2018

PaulPrice Jul 19, 2018

		@@ -1,2 +0,0 @@
		@LSST BUILD@ &&
		build_lsst @PRODUCT@ @VERSION@ @REPOVERSION@

DM-15105: Style guide fixes #138

DM-15105: Style guide fixes #138

Conversation

timj commented Jul 13, 2018

timj commented Jul 13, 2018

Choose a reason for hiding this comment

timj commented Jul 13, 2018

Choose a reason for hiding this comment

PaulPrice commented Jul 13, 2018

RobertLuptonTheGood commented Jul 14, 2018 via email

timj commented Jul 16, 2018

timj commented Jul 16, 2018

timj commented Jul 16, 2018

r-owen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timj commented Jul 16, 2018

laurenam commented Jul 16, 2018

PaulPrice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment