<div dir="ltr">Hi Paolo,<div><br></div><div>Sorry for my slow response, was quite swamped last couple of weeks...</div><div><br></div><div>A couple of comments from my side:</div><div><br></div><div>- It would be great to have a SearchIO module</div><div><br></div><div>- I don't think BioJava should be distributing OS specific binaries though. </div><div><br></div><div>- The Core module should not have dependencies on other modules (move the code from that it is dependent on into -core)</div><div><br></div><div>- Please provide some test cases, so it fits to the overall source code style of the project. Ideally these would contain support for parsing output from different Blast versions (result files could be in the test/resources ). These can also server as an example, how to use that code.</div><div><br></div><div><br></div><div>Please submit a pull request, so we can take it from there!</div><div><br></div><div>Thanks,</div><div><br></div><div>Andreas</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 9, 2015 at 9:48 AM, Paolo Pavan <span dir="ltr"><<a href="mailto:paolo.pavan@gmail.com" target="_blank">paolo.pavan@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div><div><div>Dear,<br></div>I would like to send a pull request for this.<br></div>Anyway, it is still open the issue of the new brought dependency of core from alignment module that obviously it is not ideal.<br></div>As detailed in my previous mail, it could be easily avoided by moving alignment data structure from alignment module to core, which also would be better for several reasons including avoiding problem like this in the future.<br></div>Anyway, this last refactoring is an API change and should be then considered a major version change according to semantic versioning, therefore to be added to biojava 5, sorry about that.<br><br></div>There are any issues with that? Can I do this major refactoring?<span class="HOEnZb"><font color="#888888"><br></font></span></div><span class="HOEnZb"><font color="#888888">Paolo<br></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">2015-08-30 15:32 GMT+02:00 Paolo Pavan <span dir="ltr"><<a href="mailto:paolo.pavan@gmail.com" target="_blank">paolo.pavan@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div><span></span></div><div><div><span></span></div><div><div>Thank you Jose for reviewing my code!</div><div>The run module is still not on GitHub, I can add it in case the SearchIO will be accepted. Andreas also wrote me that he is going to view it. However, I remember that unfortunately it lacked an extensive test case section (it is a not so recent project of mine). </div><div>About your other comments: I actually have fear that if we move alignment data structure this could be considered a major change (according to semantic versioning) since this is an Api change and anyone referencing this class will be forced to update his own code. Anyway also in my opinion this would be the best choice. </div><div>About the last blast format isn't supported since this work is older, but as for new parsers the way to go is already defined as I described in my previous. In case, I could face the issue. <br><br>I look forward to hearing from Andrea,</div><div>Best wishes,</div><div><br></div><div>Paolo</div><div><br><br></div><div><div><div><br><div>Inviato da iPhoneIl giorno 30/ago/2015, alle ore 13:51, Jose Manuel Duarte <<a href="mailto:jose.duarte@psi.ch" target="_blank">jose.duarte@psi.ch</a>> ha scritto:<br><br></div><blockquote type="cite"><div>
Hi Paolo<br>
<br>
Sorry for the late reply, only now I've been able to have a quick
look at your new SearchIO module and I think it looks fantastic. In
my opinion this is a great and needed feature. I would definitely
want to use it for my own projects as soon as possible. Some
comments inline below.<br>
<br>
<div>On 22.08.2015 19:26, Paolo Pavan wrote:<br>
</div>
<blockquote type="cite">
<div><span></span></div>
<div>
<div dir="ltr">
<div dir="auto">
<div><span></span></div>
<div>
<div><span></span></div>
<div>
<div><span></span></div>
<div><br>
<span>Also note that this is a required part of
another module I have written that can potentially
be of community interest: a biojava-run module, to
bless it similarly to something already listened.
This latter aims to be a generic module used to run
an analysis performed by an external program. In my
case I needed ncbi blast search. So the API was
written to declare a database of biojava Sequence
objects, pass a collection of query sequences and
retrieve in output Result objects of the SearchIO
module. </span><br>
<span>I know from previous attempt echoed in the
mailing list that the orientation of the project was
to reimplement the blast algorithm in pure Java and
I agree that it would be a great idea. But until now
this project as far as I know is late and I solved
the platform portability issue by including several
binaries for all the platforms (well, the major)
packaging all together in one jar file relying upon
this great Java facility. </span><br>
<span>Anyway, all this came later. </span><br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
The biojava-run module sounds interesting too, do you have it
anywhere in github that we can have a look at it?<br>
<br>
<blockquote type="cite">
<div>
<div dir="ltr">
<div dir="auto">
<div>
<div>
<div><span></span><br>
<span>Just to spend few technical comments on the
SearchIO module:</span><br>
<span>- included in core module since it defines a new
base data structure</span><br>
<span>- include a dependency from biojava-alignment.
This is not compulsory, it is there since the
alignment data structure is included in that
package. In my opinion, moving this important data
structure in core will solve this and avoid similar
problems in the future. This is also the reason why
I choose to add those new implemented Hits/Hsp etc
directly in core, after all search is one of the
most important tasks in bioinformatics.</span><br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I agree it only makes sense to have it in core right now. However
the dependency on biojava-alignment is not ideal. In principle the
best solution would be to move the alignment data structure to core,
I agree with you. For the time being another possibility would be to
put all the new SearchIO stuff in biojava-alignment, but that
doesn't really improve the structure of things. I'd vote for moving
the alignment data structure as a sounder and better long-term
solution. <br>
<br>
This would surely require a new minor version, so that all the new
stuff would be released as biojava 4.2.<br>
<br>
<br>
<blockquote type="cite">
<div>
<div dir="ltr">
<div dir="auto">
<div>
<div>
<div><span>- BlastXML parser is implemented in the
BlastXMLQuery class. Maybe this name it is not so
meaningful, it comes from the original class that is
still there in biojava even if it seems not so much
utilised, that I initially started to improve trying
to remain tighter to the original project. From here
also the use of the class XMLHelper and some
deprecated tags I added. From the old thread I
understood that there was not any "elective choice"
of biojava for XML parsing, but anyway the job was
already done with the XMLHelper module and so this
class came to new life.</span></div>
<div><span>- it was designed to be easy to extend: </span>add
support for a new file format a developer must just
write a single class that implements the ResultFactory
interface (<span style="background-color:rgba(255,255,255,0)">I have
implemented also a blast tabular parser to show it)</span>.
The Api for biojava user does not change, it is just:</div>
<div><span style="background-color:rgba(255,255,255,0)"> <span>SearchIO</span>
reader <span>=</span> <span>new</span> <span>SearchIO</span>(<span>new</span>
<span>File</span>(<span><span>"BlastReport.blastxml</span><span>"</span></span>),
blastResultFactory);</span></div>
<div><br>
<span>- it is possible to auto recognise file formats
relying upon standard file extension. Just try a
different constructor:</span></div>
<div><span style="background-color:rgba(255,255,255,0)">
<span>SearchIO</span> reader <span>=</span> <span>new</span> <span>SearchIO</span>(<span>new</span> <span>File</span>("BlastReport.blastxml<span><span>"</span></span>));</span><br>
<span></span><span></span></div>
<div><span><br>
</span></div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
How about the different blast XML formats? Would it work with all
the latest ones? Blast+ 2.2.31 has introduced some modifications to
the format (see <a href="http://www.ncbi.nlm.nih.gov/books/NBK131777/" target="_blank">http://www.ncbi.nlm.nih.gov/books/NBK131777/</a>)<br>
<br>
<blockquote type="cite">
<div>
<div dir="ltr">
<div dir="auto">
<div>
<div>
<div><span>If you agree that this feature would be
interesting for the project I can send a pull
request for the SearchIO part and then push on my
GitHub also the run module. </span><br>
<span></span><br>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I think it is a very nice addition, in my opinion you should go
ahead with the pull request which will make it easier for everyone
to check it out and review it for a while.<br>
<br>
Jose<br>
<br>
</div></blockquote></div></div><span><blockquote type="cite"><div><span>_______________________________________________</span><br><span>biojava-dev mailing list</span><br><span><a href="mailto:biojava-dev@mailman.open-bio.org" target="_blank">biojava-dev@mailman.open-bio.org</a></span><br><span><a href="http://mailman.open-bio.org/mailman/listinfo/biojava-dev" target="_blank">http://mailman.open-bio.org/mailman/listinfo/biojava-dev</a></span></div></blockquote></span></div></div></div></div></blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
biojava-dev mailing list<br>
<a href="mailto:biojava-dev@mailman.open-bio.org">biojava-dev@mailman.open-bio.org</a><br>
<a href="http://mailman.open-bio.org/mailman/listinfo/biojava-dev" rel="noreferrer" target="_blank">http://mailman.open-bio.org/mailman/listinfo/biojava-dev</a><br></blockquote></div><div><br></div>
</div></div>