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

Update documentations in lib/substitutions #1309

Closed
wants to merge 0 commits into from

Conversation

Bob-Chen222
Copy link
Contributor

@Bob-Chen222 Bob-Chen222 commented Feb 23, 2024

Description of changes:

  • Updated docs/doxygen/Doxyfile to include header files in lib/substitutions
  • Added documentations for parallel_tensor_pattern.h
  • Added documentations for operator_pattern.h
  • Added documentations for graph_pattern_match.h

Related Issues:

Linked Issues:


This change is Reviewable

@lockshaw lockshaw self-requested a review February 23, 2024 08:13
Copy link
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

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

If possible it would be nice to flesh out some of the important (i.e., especially data structure) documentation a bit more. I didn't have time to provide examples for all of them, but I did leave one example of the style in a comment above. Obviously not all docstrings need to be as long as the example, but it's something to consider for important parts of the code.

Also try to focus on what is the nonobvious part of the code. Currently many of the docstrings include near-exact repeats of the code (e.g., "is an enum class that") but overlook what pieces they interact with or why they exist. Assume that the reader is always going to be reading the docstring alongside the code itself, so you shouldn't be repeating anything that's part of the code, function name, etc, but should instead be trying to communicate why the thing is the way it is, what it's used for in the broader library, why it exists, etc. If there are times where the function signature is essentially self-documenting, feel free to leave very brief or even just stub docstrings--the documentation should not be there because of an obligation to document every function, but because it actually contributes something over the code itself.

Also, I didn't check any of the docstring syntax, but I'll assume you got that right 🙂

Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @Bob-Chen222)


lib/substitutions/include/substitutions/graph_pattern_match.h line 16 at r1 (raw file):

 * which consists of a node_assignment that describes how the GraphPattern node mapped to PCG node and an edge_assignment that describes
 * how the GraphPattern edge mapped to PCG edge.
 */

Suggestion:

/**
 * @struct MultiDiGraphPatternMatch
 * @brief MultiDiGraphPatternMatch describes a specific location in an OpenMultiDiGraph where a given pattern matches.
 * 
 * Given a graph and a pattern there can be zero, one, or multiple locations where it can match.
 * 
 * To provide some intuition, consider matching over strings instead of graphs: given a regex pattern "a.b" and a string "acbfadbga", there are two valid match locations: 
 * we can either match the "acb" at the beginning of the string, or the "adb" in the middle of the string.
 * MultiDiGraphPatternMatch represents the difference between the two possible locations using a bidict which maps between 
 * objects in the pattern and the corresponding objects in the matched data structure. For example, in the string example above,
 * the two matchings would be as follows:
 * "acbfadbga"   "acbfadbga"
 *  ^^^               ^^^
 *  |||               |||
 *  vvv               vvv
 * "a.b"             "a.b"
 * Of course in the context of graphs there are two types of objects to be matched: nodes and edges. 
 * As such our match consists of not one but two bidict mappings: one for nodes (node_assignment) and one for edges (edge_assignment).
 */

lib/substitutions/include/substitutions/graph_pattern_match.h line 17 at r1 (raw file):

 * how the GraphPattern edge mapped to PCG edge.
 */
struct MultiDiGraphPatternMatch {

Suggestion:

struct OpenMultiDiGraphPatternMatch

lib/substitutions/include/substitutions/graph_pattern_match.h line 80 at r1 (raw file):

 * @return std::vector<MultiDiGraphPatternMatch> 
 * 
 * @details Given a pattern and a graph, find all the valid matches between the pattern and the graph with additional conditions defined by additional_criterion.

Elaborate

Code quote:

additional conditions defined by additional_criterion.

lib/substitutions/include/substitutions/operator_pattern.h line 15 at r1 (raw file):

/**
 * @brief OperatorAttributeKey is an enum class that represents the keys of the attributes of an Operator.

Obvious from the code, remove

Code quote:

is an enum class that 

lib/substitutions/include/substitutions/operator_pattern.h line 79 at r1 (raw file):

/**
 * @brief OperatorAttributeValue is a variant that represents the concrete value of an attribute of an Operator.

Obvious from the code, remove

Code quote:

is a variant 

lib/substitutions/include/substitutions/parallel_tensor_pattern.h line 25 at r1 (raw file):

/**
 * @brief evaluate_attribute_expr evaluates the attribute expression for a given ParallelTensor

Obvious from the code.

Code quote:

evaluate_attribute_expr evaluates the attribute expression

Copy link
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Bob-Chen222)

@Bob-Chen222
Copy link
Contributor Author

Thanks Colin for your feedback. I will update docs based on your suggestions

@Bob-Chen222
Copy link
Contributor Author

I added some more documentation with some examples and more detailed explanations. I hope this one is better, and I look forward to the input!

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.

3 participants