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

Add doctest for dagascii.draw #9678

Merged
merged 1 commit into from
Jul 1, 2023

Conversation

Erotemic
Copy link
Contributor

@Erotemic Erotemic commented Jul 1, 2023

This PR is just additional docs for the dagascii.draw function. Specifically documents the return value type and it adds a doctest to illustrate what the function does.

I'm a big fan of text representations of graphs. So when I was browsing the code and saw this I got excited. But while reading the code I wondered: what does this graph look like? I went through the effort of constructing a small example and printed it for myself. Certainly a nifty way to visualize a DAG. I think it would be nice if the code was able to provide a gist of that output without requireing the user to construct an example, and I think adding a doctest here provides that.

Not sure how maintainers feel about doctests -- I see the repo has some of them, but not too many. As I learn codebases I often need to construct mwe to illustrate what a piece does for myself, so if the maintainers are interested, I'll continue submitting small PRs that add doctests like these as I generate them for myself.

@codecov
Copy link

codecov bot commented Jul 1, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (046ebb0) 90.62% compared to head (4b38d5b) 90.62%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9678   +/-   ##
=======================================
  Coverage   90.62%   90.62%           
=======================================
  Files         472      472           
  Lines       36283    36283           
  Branches     5218     5218           
=======================================
  Hits        32883    32883           
  Misses       2814     2814           
  Partials      586      586           
Impacted Files Coverage Δ
dvc/dagascii.py 91.30% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@efiop efiop merged commit e29da6e into iterative:main Jul 1, 2023
20 checks passed
@efiop
Copy link
Contributor

efiop commented Jul 1, 2023

Thanks, @Erotemic ! 🙏

We are not really against doctests, but I also can't say we've put much effort into having them everywhere. As with anything, it is really about the balance and in case of dagascii it is a rarely used internal module from many years ago, so there wasn't much impetus to touch it. It is great to see it was interesting for someone to play with it 🙂

@Erotemic
Copy link
Contributor Author

Erotemic commented Jul 1, 2023

Thanks, FYI if you haven't seen it, I recently contributed a general text-based graph visualizer to networkx. Using the same example:

>>> import networkx as nx
>>> vertices = [1, 2, 3, 4]
>>> edges = [(1, 2), (2, 3), (2, 4), (1, 4)]
>>> g = nx.DiGraph()
>>> g.add_nodes_from(vertices)
>>> g.add_edges_from(edges)
>>> nx.write_network_text(g)
╙── 1
    ├─╼ 2
    │   ├─╼ 3
    │   └─╼ 41
    └─╼  ...

This DVC variant is definately prettier, but I've been finding the conciseness of this text-based representation to be useful when debugging my dags as well as showing them to the users. The first example in the cmd-queue docs shows a place where I use it in practice.

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.

2 participants