Ed, Thanks for checking into all of this. See below for a couple of minor comments:
On Mon, Jun 22, 2009 at 7:29 PM, Ed Lee <[email protected]> wrote: > Hi Jonathan, > > Thanks for the feedback and bug fixes. > > 1. Apollo crashes/hangs when displaying sequences that contain "Ns". It >> doesn't necessarily happen immediately, but as soon as you zoom far enough >> in on a region with Ns (e.g., by using the Ctrl-Z/zoom to feature >> shortcut) >> the main display will freeze and stop updating. I traced this to an >> apparently undocumented change in DNAUtils.java, which in version 1.10.0 >> was >> capable of reverse complementing sequences with Ns, but in 1.11.0 will >> throw >> an IllegalArgumentException if given an 'N' (see DNAUtils.complement) >> > > You're absolutely right, the old code wouldn't throw an exception > if the base was not [ACGTacgt]. I've contacted the developer who made > the change to make sure that this correct behavior won't break any of > the new features. As far as I can tell, it should be OK, but figure > he might have had a reason for explicitly throwing an exception, so > we'll see. > Thanks for checking on that. It did cross my mind that the change might have been deliberate, but I couldn't find any documentation to that effect. One question I had but didn't investigate is what happens and/or should happen to characters other than a,c,t,g, and n; I only partially reverted the change, so that if the class is asked to reverse complement anything other than these 5 it will still throw an exception where it did not before. The presence of the DNAUtils.getAllowedDNACharacters() method suggests that these are the only 5 characters (times 2 for upper and lower case) that are permitted. On the other hand, I don't see any actual verification of this in the DNAUtils code, so it looks like more of a suggestion than a rule at this point. There are also some comments in there about expanding the class to the full set of IUPAC codes. 2. After fixing bug #1 there's still a problem with zooming in too far in > regions with Ns, this time an array out of bounds exception in > ScaleView.drawGaps. I'm less sure of this one because I don't remember how > coordinates are handled in Apollo, but comparing it to > Sequence.getResiduesImpl I think it's fairly clear that the loop in > drawGaps > goes 1 base too far. I'm not sure how/when this change was introduced, but > reducing the upper bound of the loop by 1 gets the gaps to render correctly > in the example sequence I'm testing with. > This is some pretty old code (I don't think I even ever touched it =P ). > As long as the change worked with all your test data, we should be fine. > I didn't test it extensively. However, even if I messed it up somehow (and I don't think I did) the worst thing that's going to happen is that the wavy red line used to indicate a gap will be slightly off. Slightly meaning that you're probably only going to notice it if you're zoomed in far enough to see the Ns anyway! So I think it's a safe patch, and it eliminates that fatal out of bounds exception (and makes the red line occupy the correct position in my test contig.) 3. (This may be the cause of Sandra's issue.) There's a logic error in > GFF3Adapter, whereby the oboParser only gets initialized when > getCurationSet() is called. This is fine if you load your original data > from GFF and then attempt to layer additional GFF annotations (via > addToCurationSet), but it's not so good if you load your original data from > a different source (chado, chadoxml, etc.) and then try to layer GFF > annotations on top. You'll get a NullPointerException on the oboParser, > from what I remember. The fix is simply to move the oboParser > initialization to a different method. > Thanks for picking that up. That's what happens when the ability to > layer GFF3 data was added after the original design =P > > 4. The NavigationBar is broken for the chado/JDBC adapter; whatever you >> tell >> it to do it just ends up reloading the same initial gene or region. >> There's >> a fix for this going into ChadoAdapter.java. >> > > I'm guessing you mean the navigation bar that allows you to move from > different loaded genomic segments? I think it was originally designed > to be used for the Indiana FlyBase servers, so I'm sure it was never > thoroughly tested out with the other data adapters. Thanks for the > changes to add the support for the Chado adapter. I don't think many > people outside of fly users were using the navigation bar, but it makes > complete sense to have support for it with the Chado adapter. > Yes, that's what I'm talking about; I should have mentioned that this was only really used for fly. I left it enabled and one of our users asked why it wasn't working, so I investigated, but it's not so much a bug as an unsupported feature. 5. The chado/JDBC option searchHitsHaveFeatLocs cannot be set to false. It > used to be supported, but appears to have been broken when the underlying > SQL was modified. Set it to false and the adapter will generate invalid > SQL. I started working on this but don't have a completed fix yet (mostly > because I realized that I don't need it for our upcoming workshop.) > If you have the fix handy (and it won't take too much work on your part) > I'll wait for your fix. Otherwise I'll investigate the issue and write > a bug fix. Let me know. > I have a counterproposal here, which is that if nobody (else) complains about this being broken in the next release you could just deprecate/remove the feature completely. I didn't check the exact dates, but I think this was broken in the 1.10.0 release too, so it may well be that nobody is using search hits without featurelocs any more; it might be one of those quirks of the old TIGR chado databases that isn't relevant any more. Cheers, Jonathan
_______________________________________________ apollo mailing list [email protected] http://mail.fruitfly.org/mailman/listinfo/apollo
