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

WIP: adding scaladoc comments for Diplomacy (Nodes.scala). #2308

Closed
wants to merge 2 commits into from

Conversation

richardxia
Copy link
Member

Related issue:

Type of change: other enhancement

Impact: no functional change

Development Phase: proposal

This is not really ready for a review, but I kind of wanted this to be an FYI to the both of you on this. I had a lot of spare cycles on my flight, and I just tried to dive right in to the Diplomacy codebase. This is speaking from the perspective of someone who doesn't really know any of our TileLink code, who doesn't really work on hardware, and who is approaching this from a much more graph theoretical/abstract functional programming types perspective.

I have to say, a lot of this code and its design is elegant in how simple it is (I am not using that sarcastically). I think the main stumbling blocks for people are probably due to how generic they are (everything is super type parameterized) and not understanding the mnemonics and metaphors that guide the naming conventions in this code.

I wanted to jot down more of the latter to help others with reading this.

Also, pulling in @jackkoenig in case if he has thoughts on how I should be representing that giant blob of comments at the beginning of the file. It doesn't exactly feel appropriate as a Scaladoc comment, since it's not about documenting individual classes or methods.

I am definitely planning to spend more time working on this.

Release Notes

@jackkoenig
Copy link
Contributor

Also, pulling in @jackkoenig in case if he has thoughts on how I should be representing that giant blob of comments at the beginning of the file. It doesn't exactly feel appropriate as a Scaladoc comment, since it's not about documenting individual classes or methods.

For overarching documentation, the standard style seems to be either wholly separate documentation, or to put it on the package, eg. scala.collection: https://www.scala-lang.org/api/2.12.10/scala/collection/index.html. It is also pretty normal to put usage notes and examples that don't apply to a particular function on the relevant [often abstract] class, eg. List: https://www.scala-lang.org/api/2.12.10/scala/collection/immutable/List.html.

Glancing at the big comment you wrote, I think it would be reasonable to put it on package diplomacy with links to more information (and examples) on classes like NodeImp.

* # Inward/Outward vs. Upward/Downward
*
* Diplomacy defines two dimensions: inward/outward and upward/downward.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

So, is it correct to say that inward/outward refer to the way edges move, and upward/downward refer to the way parameters move? Is this a helpful summary?

* # Handles
*
* Two Diplomatic nodes can be bound together using the := operator or one of
* its sibling operators. Binding is asymmetric, and the binding operation will
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you decide to put so much here about the := operator, but not go into :=* and friends? Did you just run out of time or was that a concious division? Later we talk about "ostars" without any intro

Copy link
Member Author

Choose a reason for hiding this comment

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

I was sort of thinking of mentioning the other operators, but I think that probably goes into a different section that describes the operators in more detail. My primary goal for writing this section was to understand what a Handle is, why it exists, and something to help with why it's specifically named a "handle". I'd also want to run everything I've written here by @terpstra, since everything I wrote is just me trying to "create a theory of handles" from reading the code, and I'm not sure if the design principles I post hoc derived are actually what @terpstra's goals were.

This Handle stuff is only really a concept that you'd need to understand if you were trying to deeply understand the code here, but it's not necessarily something you'd need to know as a user of a library implemented in Diplomacy. For example, anyone using the Diplomatic TileLink framework needs to understand :=, :=*, etc., but they wouldn't need to know anything about a Handle.

For what it's worth, the last part of the code that I got to on the plane while trying to was the four different NodeBinding types and their associated operators, but I had not gotten to the point of actually comprehending the code yet, since the code is recursive, meaning that in order to understand resolveStar(), you need to understand how resolveStar() works on the neighboring nodes. This is a bit challenging when you don't have internet or any prior knowledge on using any of the operators besides the plain "bind-once" := operator.

Copy link
Member

Choose a reason for hiding this comment

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

My primary goal for writing this section was to understand what a Handle is, why it exists, and something to help with why it's specifically named a "handle".

I think handle is implemented as its name.

nodeA := nodeB

will return a new handle with nodeA as outwardNode and nodeB as inwardNode.
If nodeB doesn't have a inwardNode, bind will just return a OutwardNodeHandle which only have a outwardNode nodeA.

@@ -248,6 +415,13 @@ sealed abstract class MixedNode[DI, UI, EI, BI <: Data, DO, UO, EO, BO <: Data](
val inward = this
val outward = this

/**
* Given counts of known inward and outward binding and inward and outward
* star bindings, return the resolved inward stars and outward stars.
Copy link
Contributor

Choose a reason for hiding this comment

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

what is a "star binding"? We never talked about it. Not sure if that was intentional vs running out of time to type :)

Copy link
Member Author

Choose a reason for hiding this comment

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

This was a running out of time (or really, the passing out due to being awake for 20 hours) thing. This is where I got to when I hit the "Understanding NodeBindings and running into a recursive block" described above.

@@ -560,7 +747,9 @@ class IdentityNode[D, U, EO, EI, B <: Data](imp: NodeImp[D, U, EO, EI, B])()(imp
}
}

// EphemeralNodes disappear from the final node graph
/**
* EphemeralNodes disappear from the final node graph
Copy link
Contributor

Choose a reason for hiding this comment

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

i never understood what this meant. So you put them beween two nodes and then it's like they were never there...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I didn't touch this at all besides changing the plain comment to a Scaladoc comment. I have not yet gotten to a point where I understand what an EphemeralNode is or what it should be used for.

Copy link
Contributor

@mwachs5 mwachs5 left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! Do you mind if i push directly to this branch for the Nodes which you are missing?

@richardxia
Copy link
Member Author

Thank you for doing this! Do you mind if i push directly to this branch for the Nodes which you are missing?

That works for me, but I think @sequencer has a more complete list of Scaladoc comments for the classes here, so I wonder if we should work on getting his work integrated in first?

@sequencer
Copy link
Member

Sure! I'll PR these documentation tonight.

@sequencer
Copy link
Member

comments of LazyModule are added, I'll work on Nodes.scala tomorrow.

@mwachs5 mwachs5 changed the title WIP: adding scaladoc comments for Diplomacy. WIP: adding scaladoc comments for Diplomacy (Nodes.scala). Mar 4, 2020
add my suggestions before merging this PR with the other

Co-Authored-By: Jack Koenig <koenig@sifive.com>
@sequencer
Copy link
Member

closed since #2560.

@sequencer sequencer closed this Jul 27, 2020
sequencer added a commit that referenced this pull request Aug 23, 2020
@sequencer sequencer deleted the add-scaladocs-to-diplomacy branch September 6, 2020 18:10
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.

4 participants