Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace eval; deal with inf #134

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

keflavich
Copy link
Contributor

I encountered this error:

  File "/Users/adam/repos/astrodendro/astrodendro/io/util.py", line 54, in parse_newick
    items[branch_id] = eval("{%s}" % string[start + 1:end])
  File "<string>", line 1, in <module>
NameError: name 'inf' is not defined

which occurred because my dendrogram was written with boatloads of infs int it. That's a problem with the dendrogram, and maybe it should be handled higher up - there weren't infs in my data originally, but there were nans.

More importantly, though, the use of eval is insecure and unreliable. I replaced it with ast.literal_eval so that only dictionaries containing numbers and None should be returned.

@astrofrog @ChrisBeaumont - Changing inf to None is probably not the right approach, but it's a stopgap. Can you recommend another approach?

@keflavich
Copy link
Contributor Author

Whoa I lied there were infs! But anyway dendro should deal with those somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant