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 data carrying graph types #190

Closed
wants to merge 31 commits into from
Closed

Add data carrying graph types #190

wants to merge 31 commits into from

Conversation

bwetherfield
Copy link
Member

This PR implements a Node-analog to weighted graph implementations, namely graphs where each node is assigned a piece of data. A system of protocols and structs mirror those of the ordinary graphs but with this added data carrying feature.

The PR is motivated by various use cases in the PitchSpeller modules, where we want

  • a graph structure where nodes are not only labeled but also carry Pitch values
  • a graph structure where nodes are assigned a Tendency value

In the above cases and in other usage, the data carrying graph types will give us O(1) lookups of data associated with the nodes thanks to the dictionary storage of the data.

@bwetherfield
Copy link
Member Author

@jsbean, this will be really handy to pull in while I am working on the PitchSpeller! Basically just felt I needed this... needs tests but they should be easy to pull over from *GraphTests

/// Inserts the given `node` without making any connections to other nodes contained herein.
@inlinable
public mutating func insert(_ node: Node) {
nodes.insert(node)
Copy link
Member

Choose a reason for hiding this comment

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

Indent this guy.

@jsbean
Copy link
Member

jsbean commented Oct 16, 2018

This is looking very compelling. To clarify, is the main affordance of these structures to be able to store non-Hashable values along with nodes? Have you run into this in the PitchSpeller process?

Given the following values:

let value: Int // (Hashable)
let data: String // (Hashable)

How are these two structures different?

let dg = DataGraph<Int,String>()

and

struct DataNode {
    let value: Int
    let data: String
}
extension DataNode: Equatable { }
extension DataNode: Hashable { }
let g = Graph<DataNode>()

@jsbean
Copy link
Member

jsbean commented Oct 16, 2018

The two use cases given:

• a graph structure where nodes are not only labeled but also carry Pitch values
• a graph structure where nodes are assigned a Tendency value

could potentially be implemented with the current Graph structures, but wrapping up the label and the associated data into a struct. This would still afford O(1) access.

I could be missing something crucial here, though. Lemme know, @bwetherfield !

@bwetherfield
Copy link
Member Author

I'm going to go back to NotationalModel and hopefully push something that motivates this more concretely!

@jsbean
Copy link
Member

jsbean commented Oct 16, 2018

Is NotationModel PR #123 a bit that would benefit from this?

@bwetherfield
Copy link
Member Author

bwetherfield commented Oct 16, 2018

Yes, and further NotationalModel PR #126...

So, in that PR, I've switched things from being Array-oriented to more Dictionary-oriented when it comes to Pitch. AssignedNode is unscathed. It does yield an asymptotic improvement to spelling all the pitches injected in (post-processing after the network algorithms).

But...

I haven't checked whether instead of passing around structures of the form pitches = [Int: Pitch], the same improvements can be made with something of the form pitches = [IndexedPitch] where

struct Pitch {
    let index: Index
    let value: Pitch
}

Judging from the fact that I could leave AssignedNode be (without loss), IndexedPitch may end up being just as good!

Let me know your reflections, @jsbean!

@jsbean jsbean deleted the carrier-graph branch November 17, 2018 03:17
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