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 <tlinnet@xxxxxxxxx> 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 <edward@xxxxxxxxxxxxx>:
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
<NO-REPLY.INVALID-ADDRESS@xxxxxxx> 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
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: [sr #3043] Support for NMRPipe seriesTab format *.ser