jmcbroome / BTE

Cython API for the Mutation Annotated Tree (MAT) Online Phylogenetics Toolkit
https://jmcbroome.github.io/BTE/build/html/index.html
MIT License
10 stars 5 forks source link

feat: write_newick should write to a file or be renamed #17

Closed theosanderson closed 1 year ago

theosanderson commented 1 year ago

Just my opinion, but I think there's an expectation that write_newick will take as first argument a filename and will write to there, whereas in fact it returns a string. IMO a better name for the latter function would be get_newick or similar. write_vcf and write_json do seem to write files, so the inconsistency is also a bit confusing.

As a separate issue, calling tree.write_newick("a_file.nwk") (which of course doesn't actually make any sense due to the arguments) leads to a crash due presumably to specifying a non-existing subtree (the actual first argument). It might be possible to avoid this with some sort of check for existence?

Feel free to close without action if these aren't priorities for you

jmcbroome commented 1 year ago

You're correct that this could be clearer. I will look into making this tweak as I have time. In the meantime, for anyone checking this issue, you can print newick to a file by using a file handler-

with open("out.nwk",'w+') as of:
    of.write(tree.write_newick())

I'll also look into writing some handling to avoid related crashes. Segmentation faults in interactive kernels are significantly more problematic than properly handled exceptions.