Just returned back from Roskilde Festival. I will look into it tomorrow after a mountain of emails. :-)
Best Troels Emtekær Linnet 2013/7/9 Edward d'Auvergne <[email protected]>: > Hi Troels, > > I was wondering how this patch is progressing? > > Cheers, > > Edward > > > > On 26 June 2013 17:00, Edward d'Auvergne <[email protected]> wrote: >> Hi, >> >> This looks good. Maybe I will just apply your patches one by one as >> each looks reasonable. I can apply them on my system using syntax >> such as: >> >> $ patch -p1 < 0001-Imported-the-expected-used-modules-in-lib.software.n.patch >> >> Before I apply them (which I cannot do until I return from holidays in >> the second week of July), I have a number of comments: >> >> >> patch 1 - No problems there, except for the missing '.' at the end of >> the "Progress sr #3043: ..." line in the commit message. >> >> >> patch 2 - The commit message should say rather than adding a >> docstring, that you added a stub of a function with docstring. The >> function that has no contents would be called a stub. The rest cannot >> go too wrong ;) >> >> >> patch 3 - For this one I have a few suggestions: >> >> 1) I would like to suggest an improvement to future proof this code - >> i.e. if the NMRPipe format slightly changes, then relax might still >> work. In this patch you assume fixed positions for the MODELINE and >> VARSLINE. But from the >> test_suite/shared_data/peak_lists/seriesTab.ser file, it looks like >> that these assumptions might be dangerous. For example the line with >> the 'Mode...' text is one of many REMARK lines. The line starting >> with the VARS text is after a number of REMARK lines. My worry is >> that it would be easy for nmrPipe to add more REMARK lines. Therefore >> I would suggest that we assume that there may be more than 14 header >> lines. I think this would be a fair assumption. Then most of the >> contents of this patch would be inside a loop over the file data. As >> you loop over the lines, you search for 'Mode:' and 'VARS', and then >> store the data when it is encountered. To find the number of header >> lines, have a counter for each line in the loop, then maybe if >> 'NULLSTRING' is searched for and then add 1 for the empty line (or >> search for the first data line starting with '1' and subtract 1) and >> use the 'break' statement to end the loop. Such an approach would be >> much more robust for future nmrPipe versions, and maybe also the >> current version if a user does something slightly differently. >> >> 2) All variables should be in lower case to match the relax coding >> style. Exceptions do exist, for example for parameters such as 'pA', >> but this is rare. >> >> 3) There are two empty lines at one point. >> >> 4) The third line of the commit message is unnecessary as all this >> information is already in the header line. >> >> >> patch 4 - It is best to avoid the re.match() function, and it appears >> to unused in all the patches anyway. The last line of the commit >> message could be merged into the header line. There's no need for 3 >> lines of commit messages for such a simple change. >> >> >> patch 5 - Again a number of points: >> >> 1) For Python 3, there should be spaces after all commas (at least >> the 2to3 Python 2 to 3 conversion program enforces this, so it must be >> important). >> >> 2) There are still references to Sparky present. >> >> 3) For the text "The peak intensity value " + repr(intensity) + " >> from the line " + repr(line) + " is invalid.", it is better to use the >> string formatting with %s. relax is slowly migrating to this format >> throughout, as it is faster, easier to read and easier for Python 3 >> (in some cases). >> >> 4) Again variables should be lower case (ASS_i and Z_A). >> >> 5) It is best to avoid highly Pythonic constructs such as >> enumerate(), filter, list construction using with mapping, zipping, >> for loops, etc. I.e. very Python specific ways of doing things. >> Avoiding this future-proofs the code. It is also easier on new relax >> developers who do not know Python. Keeping the constructs simple is a >> good thing. Nevertheless if the code is thoroughly checked using the >> test suite, a little bit of Pythonicness is ok. >> >> Cheers, >> >> Edward >> >> >> >> >> >> >> On 26 June 2013 15:25, Troels Emtekær Linnet <[email protected]> wrote: >>> Hi Edward. >>> >>> Just see this as a test round. :-) >>> >>> Most important is, that you can read these patch files. >>> In each patch, is also stored the commit message, which I >>> hope should be auto-read as well. >>> >>> I covered how it is possible to change the commit message. >>> I haven't tested yet, how it is possible to change the code commit. >>> >>> I guess that you rebase and amend commit. (Another option than "r") >>> git rebase -i HEAD~2 >>> >>> >>> The most problematic thing for me, was the understanding of the need to >>> create a branch from master. Add files. Create a new branch from the >>> just created branch. >>> Then change files. >>> Then do a "git format-patch PREVBRANCH" >>> >>> It's all about creating a lot of small branches, for each development step. >>> Merging, deleting them, comparing between them. >>> >>> Best >>> Troels Emtekær Linnet >>> >>> >>> 2013/6/26 Edward d'Auvergne <[email protected]>: >>>> Hi Troels, >>>> >>>> Would you be able to upload the patch either as the five original >>>> files or at least in a different format. On the Linux system I am >>>> using, I am currently unable to open RAR files. If you could use zip >>>> or tar.gz, that would be much better. On another note, why are there >>>> 5 patches and one commit message? The implementation of the function >>>> in lib.software.nmrpipe should only require one patch. Once I can >>>> read them, I'll start with the feedback. I may be able to do this >>>> tomorrow before I go on holidays next week. Also, I was wondering if >>>> you know how to use git to change the contents of a commit, as that >>>> should be possible? >>>> >>>> Cheers, >>>> >>>> Edward >>>> >>>> >>>> On 25 June 2013 19:06, Troels E. Linnet >>>> <[email protected]> wrote: >>>>> Follow-up Comment #33, sr #3043 (project relax): >>>>> >>>>> Completed NMRPipe SeriesTab reader for assignment according to SPARKY >>>>> format. >>>>> >>>>> Progress sr #3043: (https://gna.org/support/index.php?3043) Support for >>>>> NMRPipe seriesTab format *.ser >>>>> >>>>> Multiple spectra and intensity reading for NMRPipe SeriesTab formatted >>>>> file >>>>> completed. >>>>> >>>>> ----------- >>>>> Zip file with 5 patches added >>>>> >>>>> Completed according to: >>>>> http://nmr-relax.kimlinnet.dk/index.php?title=Git_asynchronous_development#Complete_test_suite >>>>> >>>>> Patches should be applied with: >>>>> patch -p1 -i >>>>> 0001-Imported-the-expected-used-modules-in-lib.software.n.patch >>>>> >>>>> patch -p1 -i >>>>> 0002-Imported-the-expected-used-modules-in-lib.software.n.patch >>>>> >>>>> >>>>> >>>>> (file #18166) >>>>> _______________________________________________________ >>>>> >>>>> Additional Item Attachment: >>>>> >>>>> File name: seriestab_patches_v1.rar Size:4 KB >>>>> >>>>> >>>>> _______________________________________________________ >>>>> >>>>> Reply to this item at: >>>>> >>>>> <http://gna.org/support/?3043> >>>>> >>>>> _______________________________________________ >>>>> Message sent via/by Gna! >>>>> http://gna.org/ >>>>> >>>>> >>>>> _______________________________________________ >>>>> relax (http://www.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://www.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
