Hi Michael,
I'm having a problem with this patch. Did you run 'svn up' before
making it? Here is some of my output:
[edward@localhost relax-1.3]$ svn info
Path: .
URL: svn+ssh://bugman@xxxxxxxxxxx/svn/relax/1.3
Repository Root: svn+ssh://bugman@xxxxxxxxxxx/svn/relax
Repository UUID: b7916896-f9f9-0310-9fe5-b3996d8957d5
Revision: 11184
Node Kind: directory
Schedule: normal
Last Changed Author: bugman
Last Changed Rev: 11175
Last Changed Date: 2010-05-03 01:05:30 +0200 (Mon, 03 May 2010)
[edward@localhost relax-1.3]$ patch -p0 < patch
patching file auto_analyses/relax_fit.py
patching file auto_analyses/dauvergne_protocol.py
Hunk #4 FAILED at 229.
1 out of 16 hunks FAILED -- saving rejects to file
auto_analyses/dauvergne_protocol.py.rej
patching file auto_analyses/noe.py
[edward@localhost relax-1.3]$
As this says, there is problem with the patching of
auto_analyses/dauvergne_protocol.py. Any ideas?
I'll make some comments about and review the patch anyway as it is not
up to standard yet. Warning, I have a lot here ;) Ok, here we go:
- def __init__(self, pipe_name='rx', seq_args=None,
file_names=None, relax_times=None, int_method='height', mc_num=500):
+ def __init__(self, filename='rx', pipe_name='rx', seq_args=None,
directory = None, file_names=None, relax_times=None,
int_method='height', mc_num=500, pdb_file=None,
unresolved='unresolved', view_plots=True, heteronuc = 'N', proton =
'H', inc = '11'):
The 'filename' arg is confusing - compare to 'file_names'. This would
better called 'output_file' or something along those lines. Same with
'directory', this could be called 'output_dir'. They should also both
be towards the end of the argument list. Next, the spacing around '='
is not to standard.
@keyword seq_args: The sequence data (file name, dir,
mol_name_col, res_num_col, res_name_col, spin_num_col, spin_name_col,
sep). These are the arguments to the sequence.read() user function,
for more information please see the documentation for that function.
+ @keyword directory: The directory, where results files are saved.
+ @type directory: str
@type seq_args: list of lists of [str, None or str,
None or int, None or int, None or int, None or int, None or int, None
or int, None or int, None or str]
The directory epydoc text has been placed in the middle of the
seq_args text. The 'view_plots' flag should also be last, as boolean
flags should always be the very last function args in Python.
# Load the sequence.
- self.interpreter.sequence.read(file=self.seq_args[0],
dir=self.seq_args[1], mol_name_col=self.seq_args[2],
res_num_col=self.seq_args[3], res_name_col=self.seq_args[4],
spin_num_col=self.seq_args[5], spin_name_col=self.seq_args[6],
sep=self.seq_args[7])
+ if self.pdb_file: # load PDB File
+ self.interpreter.structure.read_pdb(self.pdb_file)
+ generic_fns.structure.main.load_spins(spin_id='@N')
+ else:
+ self.interpreter.sequence.read(file=self.seq_args[0],
dir=self.seq_args[1], mol_name_col=self.seq_args[2],
res_num_col=self.seq_args[3], res_name_col=self.seq_args[4],
spin_num_col=self.seq_args[5], spin_name_col=self.seq_args[6],
sep=self.seq_args[7])
+
+
This is good, allowing the sequence loading from a PDB file. However
this code is not acceptable, as you have hardcoded the '@N' atom name.
No hardcoded variables will be accepted into relax - or for that
matter any software project!
# Deselect unresolved spins.
- self.interpreter.deselect.read(file=self.unresolved)
-
+ selection.desel_read(file=self.unresolved, dir=None,
spin_id_col=None, mol_name_col=None, res_num_col=1, res_name_col=None,
spin_num_col=None, spin_name_col=None, sep=None, spin_id=None,
boolean='AND', change_all=None)
+ print 'relax> deselect.read(selected residues)'
+
There is another way of doing this - allowing file objects to be
accepted by the user function. The print statement is not good - what
if the user function is renamed in the future? No one will know what
this means or where it comes from. In the prompt code, instead of:
arg_check.is_str(file, 'file name')
we just use:
arg_check.is_str_or_inst(file, 'file name')
Then the original self.interpreter.deselect.read(file=self.unresolved)
code can be used for file objects.
# Save the relaxation rates.
- self.interpreter.value.write(param='rx', file='rx.out', force=True)
+ self.interpreter.value.write(param='rx',
file=self.filename+'.out', dir=self.directory, force=True)
Here the '.out' should be added by the calling code, and not added
here. This is basic API logic. The calling code says the output file
is 'xyz', therefore the API should provide 'xyz' and not 'xyz.out'.
The GUI should add the '.out', as this is a nightmare for the
scripting UI.
# Save the program state.
- self.interpreter.state.save('rx.save', force=True)
+ self.interpreter.state.save(self.filename+'.save',
dir=self.directory, force=True)
This needs a check that self.directory is not None.
+from generic_fns import selection
These imports are not needed. See my comments above about modifying
the user functions themselves to allow for file objects.
- def __init__(self, diff_model=None, mf_models=['m0', 'm1', 'm2',
'm3', 'm4', 'm5', 'm6', 'm7', 'm8', 'm9'], local_tm_models=['tm0',
'tm1', 'tm2', 'tm3', 'tm4', 'tm5', 'tm6', 'tm7', 'tm8', 'tm9'],
pdb_file=None, seq_args=None, het_name=None, relax_data=None,
unres=None, exclude=None, bond_length=None, csa=None, hetnuc=None,
proton='1H', grid_inc=11, min_algor='newton', mc_num=500,
user_fns=None, conv_loop=True):
+ def __init__(self, save_dir=None, diff_model=None,
mf_models=['m0', 'm1', 'm2', 'm3', 'm4', 'm5', 'm6', 'm7', 'm8',
'm9'], local_tm_models=['tm0', 'tm1', 'tm2', 'tm3', 'tm4', 'tm5',
'tm6', 'tm7', 'tm8', 'tm9'], pdb_file=None, seq_args=None,
het_name=None, relax_data=None, unres=None, exclude=None,
bond_length=None, csa=None, hetnuc=None, proton='1H', grid_inc=11,
min_algor='newton', mc_num=500, user_fns=None, conv_loop=True):
The 'save_dir' arg should be to the end (but before the boolean args).
It also is not described in the epydoc text - this is quite important
in this API.
+ # Project directory (i.e. directory containing the model-free
model results and the newly generated files)
+ if save_dir:
+ self.save_dir = save_dir+sep
+ else:
+ self.save_dir = ''
+
I like this! It's a very good flexibility addition.
# Unresolved and exclude files.
- if self.unres and not isinstance(self.unres, str):
- raise RelaxError("The unres user variable '%s' is
incorrectly set. It should either be a string or None." % self.unres)
+
+ # FIXME: This is switched off as the GUI uses a relax dummy file.
+ #if self.unres and not isinstance(self.unres, str):
+ # raise RelaxError("The unres user variable '%s' is
incorrectly set. It should either be a string or None." % self.unres)
See arg_check.is_str_or_inst() for the correct way of checking for the
file or DummyFileObject objects.
+ # Files are in same directory / no directory specified
+ if self.save_dir =='':
+ dir_list = listdir(getcwd()+sep+model)
+
+ # Directory is specified
+ else:
+ dir_list = listdir(self.save_dir+model)
Here:
+ if self.save_dir =='':
should be replaced by "if not self.save_dir:"
- self.interpreter.results.write(file='results', dir=dir,
force=True)
+ self.interpreter.results.write(file=self.save_dir+'results',
dir=dir, force=True)
This is a bug! self.save_dir should be the first part of the 'dir' arg.
- self.interpreter.deselect.read(file=self.unres)
+ selection.desel_read(file=self.unres, dir=None,
spin_id_col=None, mol_name_col=None, res_num_col=1, res_name_col=None,
spin_num_col=None, spin_name_col=None, sep=None, spin_id=None,
boolean='AND', change_all=None)
+ print 'relax> deselect.read(selected residues)'
The user function should be modified instead.
- self.interpreter.value.set(self.hetnuc, 'heteronucleus')
- self.interpreter.value.set(self.proton, 'proton')
+ # Convert hetero nucleus.
+ if self.hetnuc == 'N':
+ hetnuc = '15N'
+ if self.hetnuc == 'C':
+ hetnuc = '13C'
+ self.interpreter.value.set(hetnuc,'heteronucleus')
+ self.interpreter.value.set('1H', 'proton')
+
This code is not acceptable. The 'hetnun' arg is defined as "The
heteronucleus type, i.e. '15N', '13C', etc.", therefore 'N' should not
be passed in by the calling code. The proton type is also passed in,
as it may not alway be 1H ;)
class NOE_calc:
- def __init__(self, pipe_name='noe', noe_ref = None, noe_ref_rmsd
= None, noe_sat = None, noe_sat_rmsd = None, freq = '', unresolved =
None, pdb_file = None, results_folder = None, int_method='height',
mc_num=500):
+ def __init__(self, filename='noe', seq_args=None,
pipe_name='noe', noe_ref=None, noe_ref_rmsd=None, noe_sat=None,
noe_sat_rmsd=None, unresolved=None, pdb_file=None,
results_folder=None, int_method='height', heteronuc = 'N', proton =
'H'):
The 'filename' arg could be called 'output_file' and be to the end.
The spacing around '=' is also not correct. And why is 'mc_num'
deleted? This is a bug!
- @keyword mc_num: The number of Monte Carlo simulations
to be used for error analysis at the end of the analysis.
- @type mc_num: int
+ @type heteronuc: str
+ @keyword proton: Label of proton.
+ @type proton: str
"""
Epydoc compilation '$ scons api_manual_html' fails here as there is no
'@keyword heteronuc'.
+ if self.pdb_file: # load PDB File
self.interpreter.structure.read_pdb(self.pdb_file)
generic_fns.structure.main.load_spins(spin_id='@N')
The '@N' needs to go - it should be an API arg. 'N' is not standard
in protein structures in the PDB, and we need support for the Trp
indole NH, RNA, DNA, sugars, and small molecules.
# Deselect unresolved spins.
- if self.unresolved == '':
- print ''
- else:
- self.interpreter.deselect.read(file='unresolved') #
FIXME. relax should read the list without creating a file
+ selection.desel_read(file=self.unresolved, dir=None,
spin_id_col=None, mol_name_col=None, res_num_col=1, res_name_col=None,
spin_num_col=None, spin_name_col=None, sep=None, spin_id=None,
boolean='AND', change_all=None)
+ print 'relax> deselect.read(selected residues)'
Again this requires user function modification instead.
# Save the NOEs.
- self.interpreter.value.write(param='noe',
file='noe_'+str(self.frq)+'.out', dir = self.results_folder,
force=True)
+ self.interpreter.value.write(param='noe',
file=self.filename+'.out', dir = self.results_folder, force=True)
The '.out' needs to be added by the caller, and not be performed behind the
API.
Ok, that's it for the comments. These suggestions are very important
as these changes will affect all relax users. As there are so many
issues that will hurt current users, I would very, very strongly
recommend looking at each change one by one, making one patch per
change, and sending that in. It cannot be accepted otherwise. If an
RNA person sends in a bug report about a fatal regression, then I need
to revert (permanently remove) the offending patch. Can you imagine
if I deleted every single change here because of one or two offending
lines?
Most of the changes here are quite useful, and many are required by
the GUI. So the way to get this in is to pick an idea - i.e. the
output files and directories and make just those changes to a clean
1.3 line copy. Then send that as a patch. After I check it and apply
it, you can then make the next change, and so on. We can get this
code integrated, but this patch is far too damaging for me to accept
it in its current form.
Regards,
Edward
On 10 May 2010 08:14, Michael Bieri <NO-REPLY.INVALID-ADDRESS@xxxxxxx> wrote:
Follow-up Comment #49, task #6847 (project relax):
Description for major patch:
============================
This is a major patch for the automatic analysis protocols noe.py,
relax_fit.py and dauvergne_protocol.py. The need for these changes is the
smooth integration of the GUI and the other interfaces with automatic
protocols.
Changes include:
- Project directory can be specified.
- Unresolved residues/spins are also handled using relax dummy objects.
- Sequence is either loaded from a pdb file or a sequence file. Standard is
a
sequence file, but if a PDB file is available, it will be used.
- optimized for parameter and global settings import of different
interfaces.
- Automatic display of Grace files after noe and Rx calculations can be
blocked.
- additionally, some minor bugs are fixed.
(file #9142)
_______________________________________________________
Additional Item Attachment:
File name: patch Size:24 KB
_______________________________________________________
Reply to this item at:
<http://gna.org/task/?6847>
_______________________________________________
Nachricht geschickt von/durch Gna!
http://gna.org/
_______________________________________________
relax (http://nmr-relax.com)
This is the relax-devel mailing list
relax-devel@xxxxxxx
To unsubscribe from this list, get a password
reminder, or change your subscription options,
visit the list information page at
https://mail.gna.org/listinfo/relax-devel
Re: [task #6847] The Bieri graphical user interface.