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

Acyclic graph take #601

Merged
merged 8 commits into from
Jan 22, 2021
Merged

Acyclic graph take #601

merged 8 commits into from
Jan 22, 2021

Conversation

santiagoginero
Copy link
Collaborator

@santiagoginero santiagoginero commented Jan 22, 2021

Changes

  • Created the function AcyclicGraphTake which accepts a directed, acyclic graph and a list of two vertices, returning the intersection of the in-component of the first vertex (i.e. start vertex) with the out-component of the second vertex (i.e. end vertex).
  • Function definition, tests, and documentation are all provided.

Error checking

  • The function checks for invalid inputs of the form of the graph not being directed and acyclic, the vertices not being part of the graph, incorrect argument count, among others.

Examples

2021-01-22

2021-01-22 (1)


This change is Reviewable

@santiagoginero santiagoginero added feature New functionality, or change in existing functionality wolfram language Requires Wolfram Language implementation labels Jan 22, 2021
Copy link
Owner

@maxitg maxitg left a comment

Choose a reason for hiding this comment

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

Great job! The code looks good to me. I just have a few comments about the documentation.

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @SantiagoGiner)


Documentation/SymbolsAndFunctions/UtilityFunctions/AcyclicGraphTake.md, line 1 at r1 (raw file):

###### [Symbols and Functions](/README.md#symbols-and-functions) > Utility Functions >

This page should be added to the table of contents in the main README file.


Documentation/SymbolsAndFunctions/UtilityFunctions/AcyclicGraphTake.md, line 6 at r1 (raw file):

**`AcyclicGraphTake`** gives the intersectiom of the out-component of the first vertex
with the in-component of the second vertex;

The last character should be a ":", i.e., what follows is the example for this statement.


Documentation/SymbolsAndFunctions/UtilityFunctions/AcyclicGraphTake.md, line 15 at r1 (raw file):

```

The image cuts off the label "1" at the top. Might be another weed in the rasterizer, but you can use this image for now (the width is 478.2):

___ *[Documentation/SymbolsAndFunctions/UtilityFunctions/AcyclicGraphTake.md, line 21 at r1](https://reviewable.io/reviews/maxitg/setreplace/601#-MRfDOJ22YXcDTt-38aJ:-MRfDOJ22YXcDTt-38aK:b4ex42l) ([raw file](https://github.com/maxitg/setreplace/blob/a13e4fc950ad41e28cd0e7bf36b2f53265ed7763/Documentation/SymbolsAndFunctions/UtilityFunctions/AcyclicGraphTake.md#L21)):* > ```Markdown > ``` > >

> ```

The same here, the width is 232.2, the image is:

I wonder if smaller sizes you are getting have something to do with scaling.

@santiagoginero
Copy link
Collaborator Author

Thanks! Just finished the changes.

Copy link
Owner

@maxitg maxitg left a comment

Choose a reason for hiding this comment

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

:lgtm:

When you merge the PR, make sure to copy the PR comment (the one explaining the changes in Markdown) to the commit comment (in the field that would appear after clicking the "Squash and merge" button). This way, history is easy to understand everywhere it appears.

Reviewed 4 of 4 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@santiagoginero santiagoginero merged commit d829db6 into master Jan 22, 2021
@santiagoginero santiagoginero deleted the AcyclicGraphTake branch January 22, 2021 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality, or change in existing functionality wolfram language Requires Wolfram Language implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants