Hi Edward I have also attached the original files I modified. Maybe you have to use them? Otherwise I will check tomorrow.
Cheers Edward d'Auvergne wrote: > Hi Michael, > > I'm having a problem with this patch. Did you run 'svn up' before > making it? Here is some of my output: > > [edw...@localhost relax-1.3]$ svn info > Path: . > URL: svn+ssh://[email protected]/svn/relax/1.3 > Repository Root: svn+ssh://[email protected]/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) > > [edw...@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 > [edw...@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 <[email protected]> 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 >> [email protected] >> >> 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 >> >> > > > _______________________________________________ relax (http://nmr-relax.com) This is the relax-devel mailing list [email protected] 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
