Add an EMBL file parser to BioJava #621 by NoorAldeenMB · Pull Request #713 · biojava/biojava

NoorAldeenMB · 2017-11-21T09:49:26Z

This is an implementation for first version for embl parser feature with issue #621 Add an EMBL file parser to BioJava.

josemduarte

Thanks for the pull request!
It looks to me like the EmblParser class shouldn't be called like that but rather EmblRecord or something similar, since it is the top level container for the parsed information. I'd also rather have the parse method as a static taking the file as argument and returning the EmblRecord. Javadocs explaining how the parser should be used would be nice.

josemduarte · 2017-11-27T04:26:23Z

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

+public class EmblId {
+
+
+    private String PrimaryAccession;


By convention, variable names in java should start with lower case.

sorry for that.

josemduarte · 2017-11-27T04:28:09Z

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

+        this.file = file;
+    }
+
+    public void parse() {


This method should throw an IOException instead of catching it. Otherwise the user would not be able to handle it properly in their own application.

josemduarte · 2017-11-27T04:29:49Z

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

+
+    @Test
+    public void test(){
+        File file = new File("/home/pslpt219/Desktop/Homo.dat");


This won't work for any other user. You will need to add the file to the main/resources folder and read it from there. See how it is done elsewhere in biojava. If the file is very big, please gzip it before adding it.

thank you for the comment i have moved the file into the src/test/resources.

josemduarte · 2017-11-27T04:30:36Z

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

+    }
+
+    @Test
+    public void test(){


You will need to assert something in the test, e.g. whether fields are not null and checking that values read for the fields are as expected.

sorry for that i didn't write full tests for the class but i'm working to write cases to cover more percentage of the code.

josemduarte · 2017-11-27T04:36:47Z

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

+
+/**
+ * This class should parse the data of embl file identification
+ * @author Noor Aldeen Al Mbaidin


Could you add @since 5.0.0 tag to all the new classes here? Thanks!

NoorAldeenMB · 2017-12-03T14:55:21Z

@josemduarte
thank you for the review.
I have changed the name of class from EmblParser to EmblReader that return an object of EmblRecord.

josemduarte

Thanks a lot for the changes, they look good! Just one more small issue with the test. We can merge then unless someone else wants to comment something

josemduarte · 2017-12-03T18:11:12Z

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

+
+    @Test(expected = IllegalArgumentException.class)
+    public void givenDirectoryWhenProcessEmblFileThenThrowException() throws IOException {
+        File file = new File("./src/test/resources");


This is not quite right. See for instance how it is done here:

biojava/biojava-structure/src/test/java/org/biojava/nbio/structure/io/TestNonDepositedFiles.java

Line 60 in 359dce8

InputStream inStream = new GZIPInputStream(this.getClass().getResourceAsStream("/org/biojava/nbio/structure/io/1b8g_raw.pdb.gz"));

The important thing is to use getResourceAsStream which will work in all situations (in jar packaging for instance).

josemduarte · 2017-12-03T18:11:55Z

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

+    private String databaseCrossReference;
+    private String assemblyHeader;
+    private String assemblyInformation;
+    private String CON;


This should be lower case

I renamed as constructedSequence.

josemduarte

Thank you!

josemduarte · 2017-12-04T16:46:30Z

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

+ */
+public class EmblReader {
+
+    private StringBuilder sequence = new StringBuilder("");


This could be removed, it doesn't seem to get used

josemduarte · 2017-12-04T16:46:50Z

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

+
+import java.io.*;
+import java.util.LinkedList;
+import java.util.List;


Can be removed

heuermh · 2017-12-04T18:09:01Z

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

@@ -0,0 +1,108 @@
+package org.biojava.nbio.core.sequence.io.embl;


All source code in biojava requires the license header

heuermh · 2017-12-04T18:09:51Z

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

+    private String taxonomicDivision;
+    private String sequenceLength;
+
+    public EmblId() {


This no-arg public constructor can be removed

heuermh · 2017-12-04T18:10:50Z

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

+ * @since 5.0.0
+ * @author Noor Aldeen Al Mbaidin
+ */
+public class EmblId {


If all the fields were marked final and only settable in the constructor, this class could be marked @Immutable

heuermh · 2017-12-04T18:11:29Z

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

@@ -0,0 +1,147 @@
+package org.biojava.nbio.core.sequence.io.embl;


All source code in biojava requires the license header

heuermh · 2017-12-04T18:11:55Z

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

+
+    private StringBuilder sequence = new StringBuilder("");
+
+    public EmblReader() {


This no-arg public constructor can be removed

heuermh · 2017-12-04T18:15:10Z

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

+            String lineInfo;
+            try (BufferedReader bufferedReader = new BufferedReader(fileReader)) {
+                while ((line = bufferedReader.readLine()) != null) {
+                    lineInfo = line.substring(0, 2);


What is the difference between lineInfo and lineIdentifier here? Note also that there is no check if line is long enough for this substring call not to fail.

Sorry this was a mistake , I have added validation for line length.

heuermh · 2017-12-04T18:16:59Z

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

@@ -0,0 +1,291 @@
+package org.biojava.nbio.core.sequence.io.embl;


All source code in biojava requires the license header

heuermh · 2017-12-04T18:17:54Z

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

@@ -0,0 +1,134 @@
+package org.biojava.nbio.core.sequence.io.embl;


All source code in biojava requires the license header

heuermh · 2017-12-04T18:18:08Z

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

@@ -0,0 +1,47 @@
+package org.biojava.nbio.core.sequence.io.embl;


All source code in biojava requires the license header

josemduarte · 2017-12-12T00:06:36Z

@heuermh could you approve the latest changes so that we can merge?

NoorAldeenMB · 2017-12-20T06:40:54Z

@heuermh may i ask you to review the changes ?

paolopavan · 2017-12-20T10:11:56Z

Dear Noor Aldeen Al Mbaidi, Thank you for your work, I may be a potential heavy user of your parser. I see that you have crafted an EmblRecord class. If I can add my humble opinion, it would be great if you could convey the information parsed in the standard data structure of biojava for this things, i.e. the sequence class (to be more precise, classes inheriting AbstractSequence). In this way all the sequence downstream processing out there will greatly benefit of your work. An easy parallelism can be made with Genbank sequence parsing. Look at /biojava-core/src/main/java/org/biojava/nbio/core/sequence/io/GenbankSequenceParser.java which is triggered by core/src/main/java/org/biojava/nbio/core/sequence/loader/GenbankProxySequenceReader.java Unfortunately, I know that this would be more challenging and may go beyhond what you meant, but it would be more consistent with the framework provided. Anyway, I really appreciate the already done work, thank you again. Paolo Il giorno mer 20 dic 2017 alle ore 07:40 Noor Aldeen Al Mbaidin < notifications@github.com> ha scritto:

…

@heuermh <https://github.com/heuermh> may i ask you to review the changes ? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#713 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwuq_gKrA7O4PFQ2khPtodKf7q2nh71ks5tCKv3gaJpZM4QljWO> .

NoorAldeenMB · 2017-12-27T07:56:39Z

Dear @paolopavan
thank you for your opinion, I will try to use the AbstractSequence class and upload the changes as soon as possible.

paolopavan · 2017-12-27T10:35:26Z

Great, wonderful news ! Remember to have a look to the proxy reader mechanism (eg GenbankProxySequenceReader) to link your work consistently with the project design. Il giorno mer 27 dic 2017 alle 08:56 Noor Aldeen Al Mbaidin < notifications@github.com> ha scritto:

…

Dear @paolopavan <https://github.com/paolopavan> thank you for your opinion, I will try to use the AbstractSequence class and upload the changes as soon as possible. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#713 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwuq9i32FkV3aEHqb1VIYDZROswrBIMks5tEfg4gaJpZM4QljWO> .

sbliven · 2018-02-20T14:02:33Z

I'm going to go ahead and merge this since @heuermh's requested changes have been satisfied.

@paolopavan's suggestion to return sequence objects is a good idea. We've seen that this can be a bit tricky to do in a type-safe manner (#354), so I think it's worth submitting that as a second PR.

License headers have been added. All requested changes are satisfied.

heuermh · 2018-02-20T17:58:19Z

@sbliven I have further concerns but as you say those could be addressed in a future PR.

Noor Almbaidin added 6 commits October 19, 2017 11:05

add classes of new feature parser for embl file

68dffbb

t

3dc43b4

t

fc23c95

testing embl parser

9976d2c

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

1eb16b2

first version of embl parser

d3e8e83

josemduarte requested changes Nov 27, 2017

View reviewed changes

josemduarte reviewed Nov 27, 2017

View reviewed changes

NoorAldeenMB added 2 commits December 3, 2017 16:07

pull changes request

d49b3f9

Embl Changes

79f0ed4

NoorAldeenMB added 2 commits December 3, 2017 17:05

Embl Changes

d24230a

Embl changes

141a8da

josemduarte requested changes Dec 3, 2017

View reviewed changes

using resource loader in testing.

9673da4

josemduarte approved these changes Dec 4, 2017

View reviewed changes

josemduarte reviewed Dec 4, 2017

View reviewed changes

heuermh previously requested changes Dec 4, 2017

View reviewed changes

insert license header

40ddf40

sbliven merged commit 615c03d into biojava:master Feb 20, 2018

		@@ -0,0 +1,108 @@
		package org.biojava.nbio.core.sequence.io.embl;

		@@ -0,0 +1,147 @@
		package org.biojava.nbio.core.sequence.io.embl;


		private StringBuilder sequence = new StringBuilder("");

		public EmblReader() {

		@@ -0,0 +1,291 @@
		package org.biojava.nbio.core.sequence.io.embl;

		@@ -0,0 +1,134 @@
		package org.biojava.nbio.core.sequence.io.embl;

		@@ -0,0 +1,47 @@
		package org.biojava.nbio.core.sequence.io.embl;

Conversation

NoorAldeenMB commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

NoorAldeenMB commented Dec 3, 2017

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!

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!

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

NoorAldeenMB commented Nov 21, 2017 •

edited

Loading