[Biopython-dev] [Bug 2711] GenomeDiagram.py: write() and write_to_string() are inefficient and don't check inputs

[bugzilla-daemon at portal.open-bio.org]

http://bugzilla.open-bio.org/show_bug.cgi?id=2711





------- Comment #19 from bsouthey at gmail.com  2009-01-05 16:46 EST -------
(In reply to comment #18)
> (In reply to comment #17)
> > I do not consider this bug completely fixed for multiple reasons of which my
> > patch addressed some of these prior to the creation of the _write function. I
> > do like where _write is heading as it is making cleaner and more
> > understandable code.
> > 
> > 1) I do not understand the need for the dictionary of modules 'formatdict' in
> > _write as it creates unnecessary inefficient code. The options need to be part
> > of the check for the type of output.
> 
> OK the use of a dictionary is a style thing.  You think its ugly and
> inefficient.  Leighton and I don't find it ugly.  I thought the
> if/elif/elif/else alternative you suggested was "ugly".
> 
> The argument for the type of output does get checked (by catching a KeyError
> from the dictionary).

I agree that reportlab makes any solution "ugly" because the different types
require different arguments. I agree this is partly a style issue because it is
a case of what to do first, when to do it and when to tell the user what is
missing. 

> 
> > 2) There is no indication that the output for write and write_to_string only
> > accepts uppercase. Note the _write function states this but a user will not
> > see these. I do not understand why lowercase is unacceptable. 
> 
> As part of Bug 2718, for consistency with the rest of Bio.Graphics I think we
> should after all accept either case.
> 
> > 3) The check for renderPM at start is really redundant because _write checks
> > for it (well sort of). It is also an unnecessary delay if renderPM is not
> > used. If you really must use the dictionary (which I really do not like) I
> > would suggest something like:
> > formatdict = {'PS': renderPS, 'PDF': renderPDF,'SVG': renderSVG}
> > try:
> >     from reportlab.graphics import renderPM
> >     formatdict.update({'JPG': renderPM, 'BMP': renderPM, 'GIF': renderPM,
> > 'PNG': renderPM, 'TIFF': renderPM,'TIF': renderPM})
> 
> I don't see how that would work, because unfortunately with the reportlab API,
> we must treat renderPM differently to renderPDF, renderPS and renderSVG.
> 

This just moves the renderPM import into _write and the rest of the code runs
if you add:
except:
    renderPM=None

> > The current code would show the correct options regardless of status
> > ofrenderPM. Perhaps an exception could provide a warning that renderPM
> > is not present.
> 
> Right now we do have a "helpful" exception raised when a bitmap format is
> requested and renderPM is not installed.

Again a style issue because I just find it redundant if we already know that
renderPM is not present.

> 
> > 4) There is no test for the presence of renderPM. The test function must check
> > for renderPM and should at least provide a warning if not present. Otherwise
> > this is a surprise to a user because not all options will be available.
> 
> There is an "on demand" test - via the _write function.  As Leighton has
> already pointed out, this is nasty in that it can come as a surprise to the
> user.  However, as far as I can see the alternative is an error/warning at
> import time regardless even if the user doesn't need or want bitmap output
> (i.e. Bug 2710).  The current situation strikes me as the lesser of two evils.
> 

I mean that test_GenomeDiagram should also check for renderPM and provide a
warning if not present. So if tests are run then there is some indication that
something is missing.


-- 
Configure bugmail: http://bugzilla.open-bio.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.