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

fix: changed base location for graph_from_graphdb and added tests #1732

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

Conversation

schochastics
Copy link
Contributor

Fix #1712
Downloaded the database from here: https://zenodo.org/records/11204020
and hosted it here: https://github.com/schochastics/graphsdb/

General question: should we host it somewhere else?

Copy link
Contributor

aviator-app bot commented Mar 3, 2025

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

Copy link
Contributor

@maelle maelle left a comment

Choose a reason for hiding this comment

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

good idea!

I see a typo in the file "algothitms", it could be fixed at the same time.

I wonder whether the database should be cached in an user dir. Should the argument of where to save it be exposed?

@szhorvat
Copy link
Member

szhorvat commented Mar 4, 2025

and hosted it here: https://github.com/schochastics/graphsdb/

I suggest hosting it under the igraph organization for the purposes of allowing the function to work.

In our own copy, we should mention the license and list the original authors, in addition to linking to Zenodo. I'm really glads to see that this is on Zenodo given that the MIVIA website is gone now.

@maelle

Would we be allowed to store a copy in https://github.com/igraph/igraphdata?

This dataset is way too large for the igraphdata package. I suggest using a separate repo, exactly what @schochastics did, but under the igraph organization. I believe you should be able to create a new repo, but if not let me know and I'll see if I can fix that.


Can this function operate with a local copy of the database? Would this be easy to implement, and if yes, can we include an explanation of how to do it? file:/// URL in base?


Other than the comments above, this LGTM.

@schochastics
Copy link
Contributor Author

I will try to host it under the igraph org and put all the necessary information into the README. The files have a CC license so that should all be fine. I also checked that it works with a local copy. You simply need to provide the path to the database. I will also update the docs so that this is clear

Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Do these actually work and look better? Please merge when good and green.

@schochastics schochastics force-pushed the fix-graph_from_graphdb branch from 79867f2 to 88ed1c9 Compare March 6, 2025 12:03
@schochastics schochastics enabled auto-merge (squash) March 6, 2025 12:03
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.

How useful is graph_from_graphdb()?
4 participants