Should I raise a warning, if ?-? is found for a peak?
Troels Emtekær Linnet 2013/6/26 Edward d'Auvergne <[email protected]>: > 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
