#574 Port org.biojava.bio.program.abi.ABITracer by MaxGreil · Pull Request #769 · biojava/biojava

MaxGreil · 2018-05-06T13:07:50Z

#574
Added 3 new classes: ABITrace.java, ABITracerTest.java and ABITracerCompoundSet.java.
The test file 3730.ab1 is from https://github.com/biopython/biopython/blob/master/Tests/Abi/3730.ab1.
This file was mentioned in #535.
The test data for file 3730.abi1 is from https://github.com/biopython/biopython/blob/master/Tests/Abi/test_data.

…racer

josemduarte · 2018-05-06T16:19:17Z

Nice, thank you! Looks really good. I'll submit a detail review as soon as I can.

@el1jaC could you also have a look at this when you have a minute?

Regarding tests not passing, I think it's unrelated to this. We still have problems with tests that depend on external resources, see #606.

josemduarte

Thank you again! see my comments. Most of it is about adapting style of existing code to current standards.

josemduarte · 2018-05-06T16:22:07Z

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/ABITrace.java

+    //the next three lines are the important persistent data
+    private String sequence;
+    private int A[], G[], C[], T[], Basecalls[], Qcalls[];
+    private int TraceLength, SeqLength;


Variables should start with lower case as per normal java conventions.

I know this was like that in legacy project, but it'd be a great opportunity to fix the style while porting it :)

josemduarte · 2018-05-06T16:22:50Z

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/ABITrace.java

+import org.biojava.nbio.core.exceptions.CompoundNotFoundException;
+
+/**
+ * Title: ABITrace<br><br>


Paragraph separator should be <P> instead of <br>, see https://stackoverflow.com/questions/5260368/which-tag-should-be-used-as-paragraph-separator-in-javadoc?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa

josemduarte · 2018-05-06T16:25:00Z

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/ABITrace.java

+    //of crap pre-pended to them. This constant
+    //allows ABITrace to handle that in a way that
+    //is invisible to the user.
+    private static int AbsIndexBase = 26; //The file location of the Index pointer


Constants should be static final

josemduarte · 2018-05-06T16:26:42Z

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/ABITrace.java

+     * @throws IOException if there is a problem reading from the URL.
+     * @throws IllegalArgumentException if the URL does not contain a valid ABI file.
+     */
+    public ABITrace( URL ABIFile ) throws IOException


This and the method above are very similar. It'd be nice to extract the common part to a separate method to be shared by both.

josemduarte · 2018-05-06T16:32:09Z

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/ABITrace.java

+        try {
+            DNASequenceCreator creator = new DNASequenceCreator(ABITracerCompoundSet.getABITracerCompoundSet());
+            return creator.getSequence(sequence, 0);
+        } catch (Exception e) {


No need to catch, just let the CompoundNotFoundException go through.

Catching a general Exception here could cause undesired side effects in the error handling.

If you think the CompoundNotFoundException can't happen (other than because of a bug), then it's valid to catch it (but only that exception), log a warning and not throw it.

josemduarte · 2018-05-06T16:32:56Z

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/ABITrace.java

+    /**
+     * Returns the length of the sequence (number of bases) in this trace.
+     */
+    public int getSequenceLength() {


The @return tags are missing in a few javadocs

josemduarte · 2018-05-06T16:35:43Z

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/ABITrace.java

+import java.io.InputStream;
+
+import org.biojava.nbio.core.sequence.compound.ABITracerCompoundSet;
+import org.biojava.nbio.core.sequence.compound.DNACompoundSet;


Unused import

josemduarte · 2018-05-06T16:36:54Z

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/ABITrace.java

+     * Returns the scaling factor necessary to allow all of the traces to fit vertically
+     * into the specified space.
+     *
+     * @param - the required height in pixels.


Missing param name

josemduarte · 2018-05-06T16:39:58Z

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/ABITrace.java

+            getSubArray(RecNameArray, (IndexBase + (record * 28)));
+            RecName = new String(RecNameArray);
+            if (RecName.equals("FWO_"))
+                FWO = IndexBase + (record * 28) + 20;


Any idea what this 2 "magic" numbers mean? It'd be nice to extract them to constants with some documentation as to what they mean.

I don't know what these 2 numbers stand for. What would be the best names to call them?

I'm not familiar with the format either. They seem some kind of offset, but I can't really tell.

FYI the format is described http://www6.appliedbiosystems.com/support/software_community/ABIF_File_Format.pdf

Let's leave it as is then... unless there's other suggestions, @el1jaC ?

josemduarte · 2018-05-06T16:41:57Z

biojava-core/src/test/java/org/biojava/nbio/core/sequence/io/ABITracerTest.java

+ */
+public class ABITracerTest {
+
+    private String filePath = System.getProperty("user.dir") + "/src/test/resources/3730.ab1";


This is not going to work when running as a jar. See how to do it here: https://stackoverflow.com/questions/17730005/getresourceasstream-from-junit/26638863

MaxGreil · 2018-05-07T17:04:27Z

@josemduarte

Refactored constructors ABITrace(File ABIFile) and ABITrace( URL ABIFile ) with new shared method private void ABITraceInit(BufferedInputStream bis).
Corrected all other requests.
Left request for method private void setIndex().

josemduarte

Fantastic, thanks for the changes!

After having another look I saw a couple of minor issues.

josemduarte · 2018-05-07T18:23:41Z

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/ABITrace.java

+    //allows ABITrace to handle that in a way that
+    //is invisible to the user.
+    private static final int absIndexBase = 26; //The file location of the Index pointer
+    private int IndexBase, PLOC, PCON;


IndexBase should be lower case, plus the variable should be local to the method where it is used.

josemduarte · 2018-05-07T18:24:20Z

biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/ABITrace.java

+    //This is the actual file data.
+    private byte[] traceData;
+
+    private int maximum = 0;


Doesn't look that this is being used. I seems to be a caching variable but not used properly. I'd remove it.

MaxGreil · 2018-05-07T18:35:58Z

Fixed minor changes.

josemduarte

Thank you! great job on the porting!

MaxGreil · 2018-05-08T06:58:55Z

@josemduarte
Thank you. Is the PR now ready for merge with the master branch?

josemduarte · 2018-05-08T14:16:04Z

I'd like to leave it open for a couple of days in case there are any other comments.

el1jaC · 2018-05-10T07:42:49Z

@josemduarte sorry for the late, I didn't see the tag!

Btw, I'm very happy that classes are ported to new Biojava.

When the pull request will be merged, can I add support in ABITrace class to getImageReverted, to manage RC sequences?

josemduarte · 2018-05-10T13:10:46Z

When the pull request will be merged, can I add support in ABITrace class to getImageReverted, to manage RC sequences?

Absolutely, more than welcomed!

josemduarte · 2018-05-14T23:55:34Z

@el1jaC please go ahead and submit the new pull request when you are ready. This one is merged.

el1jaC · 2018-05-15T07:11:12Z

Thank you @josemduarte .

I'll do this in the next days!

MaxGreil added 5 commits May 5, 2018 19:26

First commit for ABITracer

631ecb1

Merge branch 'master' of https://github.com/biojava/biojava into ABIT…

cb65133

…racer

ABITracer Constructor with URL

e924815

Tests ABITracerTest now running

9ecbbe0

New ABITracer class with ABITracerTest and new ABITracerCompoundSet

e30cc52

josemduarte requested changes May 6, 2018

View reviewed changes

Code reviewer requests

e1ef8a3

josemduarte requested changes May 7, 2018

View reviewed changes

Fixed minor issues

c6fa93b

josemduarte approved these changes May 7, 2018

View reviewed changes

josemduarte merged commit 2598cee into biojava:master May 11, 2018

Conversation

MaxGreil commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josemduarte commented May 6, 2018

Uh oh!

josemduarte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josemduarte May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGreil commented May 7, 2018

Uh oh!

josemduarte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGreil commented May 7, 2018

Uh oh!

josemduarte left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGreil commented May 8, 2018

Uh oh!

josemduarte commented May 8, 2018

Uh oh!

el1jaC commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josemduarte commented May 10, 2018

Uh oh!

josemduarte commented May 14, 2018

Uh oh!

el1jaC commented May 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MaxGreil commented May 6, 2018 •

edited

Loading

josemduarte May 6, 2018 •

edited

Loading

el1jaC commented May 10, 2018 •

edited

Loading