search

phbradley / conga

Clonotype Neighbor Graph Analysis
MIT License
79 stars 18 forks source link

Support environment variable for preferred SVG to PNG utility #63

Closed bbimber closed 10 months ago

bbimber commented 10 months ago

Hello -

We are running CoNGA in a docker container in a headless way on our cluster. We noticed that in some environments the PNG seq logos are kind of malformed. We think that choosing the right utility will effect this. As it stands, CoNGA basically iterates tools in a pre-defined order of priority. If 'convert' is present, it'll be used. This PR refactors the process for selecting the tools. By default it should not change behavior at all. However, it allows the user to specific the environment variable CONGA_PNG_TO_SVG_UTILITY, which could be any of: convert, inkscape, rsvg, cairosvg, or magick. If defined, it will preferentially use that tool.

phbradley commented 10 months ago

Hi there, Thanks so much for this nice addition! I think it looks great and will be very helpful. In general, this whole svg-to-png conversion thing is a big weak point of the code.

The one super-tiny thing I see is that the order of the keys in a dictionary is not guaranteed, so there is perhaps some ambiguity in what order the conversion functions will be called. An alternative would be to use an OrderedDict for CONVERT_MAP, something like this:

from collections import OrderedDict

CONVERT_MAP = OrderedDict([
    ('convert', convert_with_convert),
    ('inkscape', convert_with_inkscape),
    ('rsvg', convert_with_rsvg),
    ('cairosvg', convert_with_cairosvg),
    ('magick', convert_with_magick),
])

Here I'm following along with the creation scheme suggested in this post:

https://stackoverflow.com/questions/45114985/how-to-create-an-ordereddict-in-python

Would you be able to make something like this change and confirm that it still works for you?

Thanks again, Phil

bbimber commented 10 months ago

@phbradley: that makes sense. On my machine the key order was being respected, but python might not guarantee this, so you change is probably a good idea. Added.

phbradley commented 10 months ago

How's it going? Are you happy with the code? Shall I go ahead and merge?

Thanks again for this nice work!

bbimber commented 10 months ago

@phbradley, yes from our end i think this is good. merging would be great.

Did you have thoughts on what to do with the rhesus branch?