Skip to content

go-counting: Add single-territory function to README; Add canonical data #1195

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

Merged
merged 2 commits into from
Mar 7, 2018
Merged

go-counting: Add single-territory function to README; Add canonical data #1195

merged 2 commits into from
Mar 7, 2018

Conversation

jssander
Copy link
Contributor

@jssander jssander commented Feb 22, 2018

Closes #560

Here is a canonical data file for the go-counting exercise. I have a couple questions:

Why are some parts highlighted in red? Also should I specify that the expected territory should be a set and not an array?

"y": 1
},
"expected": {
"stone": BLACK
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple notes on syntax:

  • Values must be one of: string (with quotes), number, array ([...]), object ({ ... })
  • Values must be followed by a comma unless it is the last value in the current array/object

},
"expected": {
"stone": BLACK
"territory": [(0, 0), (0, 1), (1, 0)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Tuples are not supported by JSON.

@cmccandless
Copy link
Contributor

@jssander

Also should I specify that the expected territory should be a set and not an array?

The use of sets is an implementation detail, so arrays are better for the canonical-data.

"y": 1
},
"expected": {
"stone": BLACK
Copy link
Member

Choose a reason for hiding this comment

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

BLACK is probably hi-lighted because it should probably be in "'s and should have a trailing , on the line

},
"expected": {
"stone": BLACK
"territory": [(0, 0), (0, 1), (1, 0)]
Copy link
Member

Choose a reason for hiding this comment

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

I think this wants to be:

"territory": [[0,0], [0,1], [1,0]]

Are you trying to portray an array of tuples? I am not sure if it is really possible to convey a tuple in JSON.

@petertseng
Copy link
Member

Please edit the PR description and amend the commit message to use one of the words in https://help.github.com/articles/closing-issues-using-keywords/ and https://github.com/exercism/problem-specifications/issues/560 (#560) so that it will automatically close that issue when this PR is merged. We wouldn't want us to forget to do that.

"y": 3
},
"expected": {
"stone": WHITE
Copy link
Member

Choose a reason for hiding this comment

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

"stone": "WHITE",

},
"expected": {
"stone": WHITE
"territory": [(2, 3)]
Copy link
Member

Choose a reason for hiding this comment

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

"territory": [[2, 3]]

"y": 4
},
"expected": {
"stone": NONE
Copy link
Member

Choose a reason for hiding this comment

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

"stone": "NONE",

},
"expected": {
"stone": NONE
"territory": [(0, 3), (0, 4), (1, 4)]
Copy link
Member

@rpottsoh rpottsoh Feb 22, 2018

Choose a reason for hiding this comment

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

"territory": [[0, 3], [0, 4], [1, 4]]

"y": 1
},
"expected": {
"stone": NONE
Copy link
Member

Choose a reason for hiding this comment

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

"stone": "NONE",

"y": 1
},
"expected": {
"stone": NONE
Copy link
Member

Choose a reason for hiding this comment

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

"stone": "NONE",

"y": 5
},
"expected": {
"stone": NONE
Copy link
Member

Choose a reason for hiding this comment

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

"stone": "NONE",

],
},
"expected": {
"TerritoryBlack": []
Copy link
Member

Choose a reason for hiding this comment

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

should have a , at EOL.

},
"expected": {
"TerritoryBlack": []
"TerritoryWhite": []
Copy link
Member

@rpottsoh rpottsoh Feb 22, 2018

Choose a reason for hiding this comment

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

should have a , at EOL. "territoryWhite", use lowerCamelCase.

"expected": {
"TerritoryBlack": []
"TerritoryWhite": []
"TerritoryNone": [(0, 0)]
Copy link
Member

Choose a reason for hiding this comment

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

"territoryNone": [[0, 0]]

Note use of lowerCamelCase. Be careful of the indentation too.

"TerritoryBlack": [(0, 0), (0, 1)]
"TerritoryWhite": [(3, 0), (3, 1)]
"TerritoryNone": []
}
Copy link
Member

Choose a reason for hiding this comment

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

"expected": {
  "territoryBlack": [[0, 0], [0, 1]],
  "territoryWhite": [[3, 0], [3, 1]],
  "territoryNone": []
}

},
"expected": {
"stone": NONE
"territory": [(2, 7), (2, 8), (1, 8), (0, 8), (0, 7)]
Copy link
Member

Choose a reason for hiding this comment

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

"stone": "NONE",
"territory": [[2, 7], [2, 8], [1, 8], [0, 8], [0, 7]]

@rpottsoh
Copy link
Member

problem description.

@rpottsoh rpottsoh changed the title add go-counting canonical data file go-counting: add canonical data Feb 22, 2018
@jssander
Copy link
Contributor Author

Thanks for all the comments, I'll make all the changes. Sorry for all the problems.

@rpottsoh
Copy link
Member

@jssander this looks like a good start, thanks for taking on the challenge and deciding to help out.

I have created a gist of your canonical data file. The left-hand pane is the JSON schema used by Exercism to confirm that canonical data files are formatted properly. In the right-hand pane is your submitted JSON. Feel free to paste your JSON into this window as you go to check it against the schema. The same check is also made when you commit.

@rpottsoh rpottsoh changed the title go-counting: add canonical data go-counting: Implement canonical-data.json Feb 22, 2018
@rpottsoh
Copy link
Member

rpottsoh commented Feb 22, 2018 via email

@petertseng
Copy link
Member

I plan to verify with code similar to the following.

require 'json'
require_relative '../../verify'

json = JSON.parse(File.read(File.join(__dir__, 'canonical-data.json')))

cases = by_property(json['cases'], %w(territory territories))

class UnionFind
  def initialize(things)
    @parent = things.map { |x| [x, x] }.to_h
    @rank = things.map { |x| [x, 0] }.to_h
    @frozen = false
  end

  def keys
    @parent.keys
  end

  def union(x, y)
    raise FrozenError if @frozen
    xp = find(x)
    yp = find(y)

    return if xp == yp

    if @rank.fetch(xp) < @rank.fetch(yp)
      @parent[xp] = yp
    elsif @rank.fetch(xp) > @rank.fetch(yp)
      @parent[yp] = xp
    else
      @parent[yp] = xp
      @rank[xp] += 1
    end
  end

  def find(x)
    @parent[x] = find(@parent.fetch(x)) if @parent.fetch(x) != x
    @parent.fetch(x)
  end

  def freeze
    @frozen = true
    self
  end
end

module EachGridPair refine Array do
  def each_grid_pair
    each_with_index { |row, y|
      row.each_char.with_index { |c, x|
        [[y + 1, x], [y, x + 1]].each { |y2, x2|
          next unless (c2 = self[y2]&.[](x2))
          yield c, [y, x], c2, [y2, x2]
        }
      }
    }
  end
end end

using EachGridPair

def territories(board)
  uf = UnionFind.new(board.each_with_index.flat_map { |row, y| row.size.times.map { |x| [y, x] } })
  board.each_grid_pair { |c1, place1, c2, place2|
    uf.union(place1, place2) if c1 == ' ' && c2 == ' '
  }
  uf.freeze
  regions = uf.keys.group_by { |k| uf.find(k) }
  colours = regions.keys.map { |k| [k, [false, false]] }.to_h
  board.each_grid_pair { |c1, place1, c2, place2|
    next if (c1 == ' ') == (c2 == ' ')
    place = c1 == ' ' ? place1 : place2
    colour = [c1, c2].sort.last == ?B ? 0 : 1
    colours.fetch(uf.find(place))[colour] = true
  }
  colours.transform_values! { |black, white|
    (black && !white ? 'BLACK' : white && !black ? 'WHITE' : nil).freeze
  }
  regions.map { |parent, v|
    y, x = parent
    [parent, {
      # TODO: call this colour instead?
      'stone' => colours.fetch(parent),
      'territory' => v,
    }] if board[y][x] == ' '
  }.compact.to_h.freeze
end

verify(cases['territory'], property: 'territory') { |i, _|
  territories(i['board']).values.find { |t|
    t['territory'].include?([i[?y], i[?x]])
  }
}

verify(cases['territories'], property: 'territories') { |i, _|
  # TODO: Type subject to discussion of multiple territories of a given colour.
  territories(i['board']).values
}

If for some reason I should become unavailable, feel free to verify in my stead. I would highly suggest that verification be performed before merging.

@jssander
Copy link
Contributor Author

I think I made all the changes mentioned, but it still doesn't seem to be proper JSON.

"TerritoryNone": [(0, 0)]
"territoryBlack": [],
"territoryWhite": [],
"territoryNone": [[0, 0]]
Copy link
Member

Choose a reason for hiding this comment

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

Watch the indents on the "territoryNone":'s. they should be the same as the two above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this why it is not valid JSON? Or is there some other problem?

@rpottsoh
Copy link
Member

It appears as though you have a mix of tabs and spaces to for indenting. It would be easier to read through the file if the tabs were replaced with a sufficient number of spaces. Tabs mean different things in different editors, it isn't always the same number of spaces.

"input": {
"board": [
" "
],
Copy link
Member

Choose a reason for hiding this comment

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

, not necessary.
Try www.jsonlint.com. It will help point out where the syntax is wrong.

"board": [
" BW ",
" BW "
],
Copy link
Member

@rpottsoh rpottsoh Feb 23, 2018

Choose a reason for hiding this comment

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

, not needed.

@jssander
Copy link
Contributor Author

Thanks for all the help and patience. It passes the checks now.

"description": "Black corner territory on 5x5 board",
"property": "territory",
"input": {
"board": [
Copy link
Member

Choose a reason for hiding this comment

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

I realise that I gave bad advice. Since this is a k/v pair under input, I would indent the board line and all the strings making up the board two spaces in

"B W B",
" W W ",
" W "
],
Copy link
Member

Choose a reason for hiding this comment

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

I would align this with the same column that "board" is; same with all other closing bracket lines

"input": {
  "board": [
    "stuff"
  ],
  "x": 0,
}

@petertseng
Copy link
Member

I haven't put in the comment saying which coordinate goes first (x or y). Would you like me to do that

Whichever of [y, x] or [x, y] you choose, I think it will be necessary to put a comment in, yes. So that it is clear to readers of this file.

and change it so it goes [y, x] instead of [x, y]?

I think last time I argued that [y, x] matches how a computer would index into the board, right? On the other hand, I think I also recall from education that [x, y] more closely matches the conventional way humans choose to designate a coordinate in 2d space? It is not a perfect analogy though since I think in such a conventional coordinate system y is increasing as you go up, but in this file y is increasing as you go down.

Unless someone comes forward with more arguments as to whether it is desirable to perfer one or the other, I don't object to either. Maybe someone wishes to retrieve some arguments from #313 .

@jssander
Copy link
Contributor Author

I left it as [x, y], but put in a comment about it. I also changed the indentation.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

Great stuff.

I am wondering whether the usage of the term "territory" is a little inconsistent. In the territory property, we consider it a contiguous region of spaces surrounded by stones. In the territories property, we consider it the union of all such contiguous regions, such that they are no longer contiguous. Would someone give me a way of thinking about this that makes everything make sense as-is?

If not, then I would think the expected result for all territories cases should change to be one of the two possibilities described in #1195 (comment) .

To give meaning to this discussion, I think a board such as [" B "] should be added as a territories test case; it is useful because the territory for a given player is two regions which aren't connected to each other, something not yet seen in any territories case.

}
},
{
"description": "Invalid coordinate on 5x5 board",
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to explain in what way the coordinate is invalid? Here, it is because X is too low.

}
},
{
"description": "Invalid coordinate #2 on 5x5 board",
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to explain in what way the coordinate is invalid, so that it is clearer than "#2"? Here, it is because Y is too high.

"property": "territories",
"input": {
"board": [
" "
Copy link
Member

Choose a reason for hiding this comment

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

let's re-indent to match the other boards

}
},
{
"description": "Open corner territory on 9x9 board",
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure what this case tests that (for territory) that any previous cases don't. I know the board is larger, but I don't think that adds any extra considerations to the algorithm. May I ask if any insight can be gleaned from any of the four implementing tracks (http://exercism.io/contribute/canonical-data/go-counting C# F# Haskell Python) why they have this test, what it does for them?

If there is nothing new being tested, I would actually think it should be removed.

@jssander
Copy link
Contributor Author

Ok, I remove the 9x9 board test, fixed an indentation, made the two invalid coordinate test case descriptions clearer, and added a test case for the " B " board. I haven't done anything about the "territory" term problem yet.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

added a test case for the " B " board

Fantastic, thank you! With this case, we can more easily gather opinions on what the answer should be. I see a few choices:

  1. The existing answer: Combine all territories into one, without regard to whether they are connected:
{
  "territoryBlack": [[0, 0], [2, 0]],
  "territoryWhite": [],
  "territoryNone": []
}
  1. Group contiguous regions into their own lists:
{
  "territoryBlack": [[[0, 0]], [[2, 0]]],
  "territoryWhite": [],
  "territoryNone": []
}
  1. Use an array whose element type is the same as the expected answer to territory
[
  {
    "owner": "BLACK",
    "territory": [[0, 0]]
  },
  {
    "owner": "BLACK",
    "territory": [[2, 0]]
  }
]

Warning: If nobody expresses an opinion in a week, I will Approve (in the GitHub sense!!) any correct submission of any of the above three choices. If you care which choice is used, you must speak up.

},
"expected": {
"owner": "NONE",
"territory": [[2, 7], [2, 8], [1, 8], [0, 8], [0, 7]]
"territoryBlack": [[0, 0], [0, 2]],
Copy link
Member

Choose a reason for hiding this comment

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

inconsistent: This uses [y, x] coordinate instead of [x, y], since a y coordinate of 2 does not exist (the x coordinate should be 2).

@petertseng
Copy link
Member

petertseng commented Mar 1, 2018

I prepared code to verify any of the three possible expected types, so as before I will leave instructions to verify this PR in case I become unable to do so: Someone who wants to verify this PR should replace the last four lines of the verify script and specify the output type we decided to use.

The current type in the submitted JSON is by_owner_combined. (As I pointed out in my earlier review, the currently submitted JSON does not verify, so I can't Approve yet)

verify(cases['territories'], property: 'territories') { |i, _|
  default = {
    'territoryBlack' => [],
    'territoryWhite' => [],
    'territoryNone' => [],
  }

  case which_output_type_should_we_use = :by_owner_combined
  when :array_of_territory
    territories(i['board'])
  when :by_owner
    default.merge(territories(i['board']).group_by { |t| t['owner'] }.map { |k, vs|
      ['territory' + k.capitalize, vs.map { |t| t['territory'] }.sort.freeze]
    }.to_h)
  when :by_owner_combined
    default.merge(territories(i['board']).group_by { |t| t['owner'] }.map { |k, vs|
      ['territory' + k.capitalize, vs.flat_map { |t| t['territory'] }.sort.freeze]
    }.to_h)
  end
}

@jssander
Copy link
Contributor Author

jssander commented Mar 3, 2018

Ok I fixed the x/y coordinate pair order for one of the tests and added tests for x and y being too high or too low.

@rpottsoh
Copy link
Member

rpottsoh commented Mar 3, 2018

Sorry for bringing it up so late. The change to description.md should probably be in its own commit and / or the title of the PR should be updated as it is no longer just implementing canonical test data for this exercise. Perhaps the title could state that this PR is updating the documentation for this exercise. The PR should only be attempting to accomplish one task.

}
},
{
"description": "Invalid because Y is too high for 5x5 board",
Copy link
Member

Choose a reason for hiding this comment

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

This looks good, thanks for making this change.

@jssander jssander changed the title go-counting: Implement canonical-data.json go-counting: Updating documentation Mar 3, 2018
@jssander
Copy link
Contributor Author

jssander commented Mar 3, 2018

I updated the title of the PR. Do you still want me to put the changes to description.md in a different commit? Can I keep it in the same PR?

@rpottsoh
Copy link
Member

rpottsoh commented Mar 3, 2018

I think it is ok as you have things now. I wouldn't worry about it unless any one else has a different opinion. Thanks for updating the PR title.

@petertseng petertseng dismissed their stale review March 4, 2018 00:09

the JSON file now verifies.

@petertseng
Copy link
Member

This file now verifies. This means that my Request for Changes is hereby dismissed.

Reminder: In a little over 3 days, it will have been one week since I submitted #1195 (review) which asks for opinions on which expected type to use for territories. Reviewers have until that time to express an opinion. If no opinion is expressed, I will make the choice of "arbitrary i.e. whatever the author of the JSON file chooses" which means I will Approve this PR in its current state.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

As discussed earlier, I choose "arbitrary; whatever consistent and correct answer the submitter chooses" for the type of territories.

@petertseng
Copy link
Member

I hope you will not consider it presumptuous that I dismiss #1195 (review). It should be easy to see why I am doing so: The review mentions invalid JSON syntax, and the syntax is valid today.

@petertseng petertseng dismissed cmccandless’s stale review March 7, 2018 19:00

I hope you will not consider it presumptuous that I dismiss #1195 (review). It should be easy to see why I am doing so: The review mentions invalid JSON syntax, and the syntax is valid today.

Copy link
Member

@rpottsoh rpottsoh left a comment

Choose a reason for hiding this comment

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

Thanks again @jssander 👍

@petertseng petertseng merged commit afbd817 into exercism:master Mar 7, 2018
@petertseng
Copy link
Member

Great job, thank you.

@jssander
Copy link
Contributor Author

jssander commented Mar 7, 2018

You're welcome. Thank you for merging.

}
},
{
"description": "A stone and not a territory on 5x5 board",
Copy link
Contributor

Choose a reason for hiding this comment

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

@jssander @petertseng I know this PR has already been merged, but should this be "A stone is not a territory..."?

Copy link
Member

Choose a reason for hiding this comment

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

If you want. I admit I don't see a reason to prefer one or the other, but that could just be because of English not being my first language. I'm happy to defer to others on this matter.

@petertseng petertseng changed the title go-counting: Updating documentation go-counting: Add single-territory function to README; Add canonical data Mar 18, 2018
petertseng added a commit to exercism/haskell that referenced this pull request Mar 18, 2018
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