Hi Michael,
Here are some comments about the relax_gui.py file. Most is
formatting for now. I'll break it into numbered sections for
reference. Note that each change should probably be a separate patch
file with separate commit message describing the change in detail.
Commits should be relatively small for this type of development.
These comments are general and apply to the rest of the relax code
base, and to all code all relax developers write. Sorry for the
length, but this should help with all your code. I haven't made major
comments about the code structure yet, this just covers formatting and
basic structure.
Cheers,
Edward
1) A module docstring would be useful at the very top (after the
copyright message). Maybe a bit like:
# relax module docstring.
"""The Bieri Graphical User Interface for relax."""
2) Imports.
2a) The 'res' imports should be shifted to the relax module section.
2b) The python module imports should be alphabetical.
2c) The "from xxx import *" statement should never be used. * needs
to be replaced by only those module objects directly used in
relax_gui.py.
3) Global variables should be all capitalised. These are more like
constants. There should be spacing after all commas. See 5) and 11).
4) Field strength defaults in the variables. This is not for
changing right now, but I would like to help you design the interface
so that you can have say 5 field strengths. And maybe have some data
missing, i.e. you collect on 500, 600, 700, 800, and 900, but the 500
and 600 NOE values are missing. This will eliminate the relaxation
related variables.
5) Preparative formatting for Python3. See the file
'docs/2to3_checklist'. On each file, execute:
$ 2to3 -w \
-f buffer \
-f idioms \
-f set_literal \
-f ws_comma \
-x except \
-x import \
-x imports \
-x long \
-x numliterals \
-x xrange \
my_file.py
This should be one patch, with all files updated. This will pick up
the missing space after commas.
6) Function docstrings are missing. See other parts of relax for how
this is done, e.g. in specifc_fns/model_free/main.py. Epydoc
formatting is required. The comment above the function should be the
basis of the docstring, i.e. move it into a docstring. See 9) for
example.
7) Functions should be alphabetically arranged.
8) Spacing is incorrect within the functions, classes, loops, etc.
There should only be 4 space characters for each indent level. See 9)
for example.
9) Comment frequency is far too low. In relax there is a policy for
commenting everything. As an example:
-----
#Results
def see_results(openfile):
if '.agr' in openfile:
os.system('xmgrace ' + openfile + ' &')
if '.txt' in openfile:
os.system('gedit ' + openfile + ' &')
if '.pml' in openfile:
os.system('pymol ' + openfile + ' &')
-----
def see_results(file_name):
"""Display the results through an external program.
The following programs are used to view the results:
- Grace, a plotting program
(http://plasma-gate.weizmann.ac.il/Grace/).
- Gedit, a text editor (http://projects.gnome.org/gedit/).
- PyMOL, a molecular viewer (http://pymol.sourceforge.net/).
@param file_name: The name of the results file to open by the
external program.
@type file_name: str
"""
# View the results as a Grace plot.
if '.agr' in file_name:
os.system('xmgrace %s &' % file_name)
# Open the results in the Gedit text editor.
if '.txt' in file_name:
os.system('gedit %s &' % file_name)
# Display the results on the molecule structure.
if '.pml' in file_name:
os.system('pymol %s &' % file_name)
-----
The idea here is to describe what the code does in basic English
terms. The reason for this is so that non-coders can help contribute
to relax. It helps them understand and follow the flow and execution
of the code. The more explanations in comments given, the better!
10) create_save_file() should be better formatted. The lists and
strings at the start are a debugging nightmare. Try something like:
-----
# NOE
savenoe1 = [str(self.nmrfreq_value_noe1.GetValue()),
str(self.noe_sat_1.GetValue()), str(self.noe_sat_err_1.GetValue()),
str(self.noe_ref_1.GetValue()), str(self.noe_ref_err_1.GetValue()),
str(self.structure_noe1.GetValue()),
replace(str(self.unres_noe1.GetValue()), ',',';'),
str(self.res_noe1.GetValue())]
-----
# NOE
savenoe1 = [str(self.nmrfreq_value_noe1.GetValue()),
str(self.noe_sat_1.GetValue()),
str(self.noe_sat_err_1.GetValue()),
str(self.noe_ref_1.GetValue()),
str(self.noe_ref_err_1.GetValue()),
str(self.structure_noe1.GetValue()),
replace(str(self.unres_noe1.GetValue()),
',',';'),
str(self.res_noe1.GetValue())
]
----
And for the strings:
-----
#T1
t1_list_1 = str(self.t1_list_1.GetLabel()) + ', ' +
str(self.t1_list_2.GetLabel()) + ', ' + str(self.t1_list_3.GetLabel())
+ ', ' + str(self.t1_list_4.GetLabel()) + ', ' +
str(self.t1_list_5.GetLabel()) + ', ' + str(self.t1_list_6.GetLabel())
+ ', ' + str(self.t1_list_7.GetLabel()) + ', ' +
str(self.t1_list_8.GetLabel()) + ', ' + str(self.t1_list_9.GetLabel())
+ ', ' + str(self.t1_list_10.GetLabel()) + ', ' +
str(self.t1_list_11.GetLabel()) + ', ' +
str(self.t1_list_12.GetLabel()) + ', ' +
str(self.t1_list_1_copy_11.GetLabel()) + ', ' +
str(self.t1_list_14.GetLabel())
-----
#T1
t1_list_1 = ("%s, "*13 + "%s") %
str(self.t1_list_1.GetLabel())
str(self.t1_list_2.GetLabel())
str(self.t1_list_3.GetLabel())
str(self.t1_list_4.GetLabel())
str(self.t1_list_5.GetLabel())
str(self.t1_list_6.GetLabel())
str(self.t1_list_7.GetLabel())
str(self.t1_list_8.GetLabel())
str(self.t1_list_9.GetLabel())
str(self.t1_list_10.GetLabel())
str(self.t1_list_11.GetLabel())
str(self.t1_list_12.GetLabel())
str(self.t1_list_1_copy_11.GetLabel())
str(self.t1_list_14.GetLabel())
-----
11) Formatting is a mess. This is related to 8). For example the
white space at the end of lines should be removed. All problems are
identified by running in the base directory the command:
$ ./scripts/code_validator gui_bieri/relax_gui.py
The output of this script will be very useful for cleaning the code up
to meet relax's high standards.
12) Statements testing booleans (True and False) can been simplified.
E.g. "if self.m0.GetValue():" instead of "if self.m0.GetValue() ==
True:".
13) The methods of the class "main" are a bit heavy. __init__(),
__set_properties(), and __do_layout() have huge amounts of code
without comments. I would consider breaking these into small methods
and vigorously commenting as to what GUI elements you are building,
and what everything is doing. This is very hard for me to follow, so
a non-coder would be totally lost in there.
14) The class "main" should be changed to "Main". This will be
identified by the code validator script in 11).
15) The class "Model" is defined within the 'start_model_free()'
function. This should never be done. Functions and classes should be
in the scope of the module, not the functions, classes or methods of a
module (rare exceptions exist, but not for something like this). This
class could probably go into its own module.
15) This module is 4126 lines long. I would recommend splitting it
up into smaller components related to the various functions. I.e. the
class "main" could go in it's own module (or 2).
Formatting of the Bieri GUI relax_gui.py file.