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

Graph[] #600

Merged
merged 27 commits into from
Aug 29, 2020
Merged

Graph[] #600

merged 27 commits into from
Aug 29, 2020

Conversation

poke1024
Copy link
Contributor

Still a work in progress, but already usable.

sn6uv
sn6uv previously requested changes Sep 30, 2016
Copy link
Member

@sn6uv sn6uv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good. I'm very happy to see graphs being finally added.

Quite a few missing edge cases, in particular the empty graph.

It's not clear which parts of networkx require numpy, certainly some of the layout stuff does. A defensive approach would be to require numpy for _NetworkXBuiltin but maybe we don't need to since most of it works without numpy.

mathics/core/parser/operators.py Outdated Show resolved Hide resolved
mathics/builtin/graphs.py Outdated Show resolved Hide resolved
mathics/builtin/graphs.py Outdated Show resolved Hide resolved
mathics/builtin/graphs.py Show resolved Hide resolved


class UndirectedEdge(Builtin):
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs missing. Maybe a good place to test the parsing of <->.

mathics/builtin/graphs.py Outdated Show resolved Hide resolved
mathics/builtin/graphs.py Show resolved Hide resolved
mathics/builtin/graphs.py Show resolved Hide resolved
mathics/builtin/graphs.py Show resolved Hide resolved
mathics/builtin/graphs.py Show resolved Hide resolved
@sn6uv
Copy link
Member

sn6uv commented Sep 30, 2016

I'd tentatively like to suggest including this in 1.0 but if it's not ready then that's okay too.

#> PathGraphQ[Graph[{1 -> 2, 2 -> 1}]]
= True
#> PathGraphQ[Graph[{}]]
= False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is doubled up

@sn6uv
Copy link
Member

sn6uv commented Oct 1, 2016

Note: Now that #604 is merged the graph tests should now run on Travis.

@sn6uv
Copy link
Member

sn6uv commented Oct 3, 2016

Let's leave this for the next release.

@GarkGarcia
Copy link
Contributor

@poke1024 Hey, would you be willing to rebase this branch so that we can finally merge it?

@GarkGarcia
Copy link
Contributor

@poke1024 Thanks for the rebase! It looks like most failing tests are caused by AttributeErrors, any idea why?

@poke1024
Copy link
Contributor Author

@GarkGarcia I'm currently looking into this.

@GarkGarcia GarkGarcia dismissed sn6uv’s stale review August 29, 2020 15:30

All tests have passed, so I believe we're good.

@GarkGarcia GarkGarcia merged commit f079e5c into mathics:master Aug 29, 2020
@GarkGarcia
Copy link
Contributor

Great! Thanks for your contribution!

@GarkGarcia GarkGarcia removed this from the 1.1 milestone Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants