[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 #18 from biopython-bugzilla at maubp.freeserve.co.uk  2009-01-05 11:56 EST -------
(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).

> 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.

> 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.

> 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.

> 5) The installation documentation must also indicate that renderPM is
> optional and also how to install the renderPM module.

Yes, we should indicate renderPM is optional.  Updating our documentation to
cover GenomeDiagram is still pending on Bug 2671.


-- 
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.