New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#574 Port org.biojava.bio.program.abi.ABITracer #769
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you again! see my comments. Most of it is about adapting style of existing code to current standards.
//the next three lines are the important persistent data | ||
private String sequence; | ||
private int A[], G[], C[], T[], Basecalls[], Qcalls[]; | ||
private int TraceLength, SeqLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
import org.biojava.nbio.core.exceptions.CompoundNotFoundException; | ||
|
||
/** | ||
* Title: ABITrace<br><br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
//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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constants should be static final
* @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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
try { | ||
DNASequenceCreator creator = new DNASequenceCreator(ABITracerCompoundSet.getABITracerCompoundSet()); | ||
return creator.getSequence(sequence, 0); | ||
} catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/** | ||
* Returns the length of the sequence (number of bases) in this trace. | ||
*/ | ||
public int getSequenceLength() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @return
tags are missing in a few javadocs
import java.io.InputStream; | ||
|
||
import org.biojava.nbio.core.sequence.compound.ABITracerCompoundSet; | ||
import org.biojava.nbio.core.sequence.compound.DNACompoundSet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import
* Returns the scaling factor necessary to allow all of the traces to fit vertically | ||
* into the specified space. | ||
* | ||
* @param - the required height in pixels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing param name
getSubArray(RecNameArray, (IndexBase + (record * 28))); | ||
RecName = new String(RecNameArray); | ||
if (RecName.equals("FWO_")) | ||
FWO = IndexBase + (record * 28) + 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what these 2 numbers stand for. What would be the best names to call them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
*/ | ||
public class ABITracerTest { | ||
|
||
private String filePath = System.getProperty("user.dir") + "/src/test/resources/3730.ab1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic, thanks for the changes!
After having another look I saw a couple of minor issues.
//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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IndexBase should be lower case, plus the variable should be local to the method where it is used.
//This is the actual file data. | ||
private byte[] traceData; | ||
|
||
private int maximum = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look that this is being used. I seems to be a caching variable but not used properly. I'd remove it.
Fixed minor changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! great job on the porting!
@josemduarte |
I'd like to leave it open for a couple of days in case there are any other comments. |
@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? |
Absolutely, more than welcomed! |
@el1jaC please go ahead and submit the new pull request when you are ready. This one is merged. |
Thank you @josemduarte . I'll do this in the next days! |
#574
Added 3 new classes:
ABITrace.java
,ABITracerTest.java
andABITracerCompoundSet.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.