Hi, Files should not be used to send the changes as this can lead to accidental loss of recent code. Patches or commits (which are patches as well) is the way code changes should be communicated. Nevertheless, exactly the same problems as described in full detail in my previous message (https://mail.gna.org/public/relax-devel/2010-05/msg00006.html) exist within these files.
Regards, Edward On 10 May 2010 11:32, Michael Bieri <[email protected]> wrote: > 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 > _______________________________________________ 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
