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

saddle-points: add canonical data #756

Merged
merged 1 commit into from
May 1, 2017
Merged

Conversation

stkent
Copy link
Contributor

@stkent stkent commented Apr 17, 2017

Closes #582.

Notes:

  • Per Intermediate functions: To test or not to test? discussions#41, the row/column identification tests found in several tracks represent testing intermediate implementation details and should be removed.
  • Larger matrices have been removed from the tests; they did not exercise anything that was not already covered (except possibly for differing matrix dimensions?)
  • Readme example is included as the first exercise; the majority of tracks already test this and it's a nice start point.
  • Empty example is included; only a few tracks currently test this, but it's a nice boundary case.
  • Multiple saddle points test uses the more-common matrix as found in e.g. xcsharp. xpython and xscala will need to update their multiple saddle points tests.
  • Example with saddle point in the lower right matrix entry has been added to check for off-by-one errors. This is new for all tracks.
  • Example with no saddle points has been expanded from 2x2 matrix to 3x3 matrix to give implementers more of a hint as to how one might go about constructing such a matrix. This is new for all tracks.
  • No check for invalid matrix size is included in the canonical suite; can be included if desired, in which case I'll pull the xpython input for that test.

@stkent stkent force-pushed the saddle-points-canonical-data branch from 4e32526 to 5a046fc Compare April 17, 2017 01:31
"input": [
[]
],
"expected": []
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be null instead of an empty array? The README indicates it does:

if the input is valid but there is no result for the input, the value at "expected" should be null.

Copy link
Member

Choose a reason for hiding this comment

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

In my interpretation, the empty result is a result, therefore it is not no result. [] makes more sense than null here.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, understood

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice spot, my bad. On reflection, it would seem that this is a case where it makes sense to update the description; I'll push a commit with that change this weekend :o)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ErikSchierboom forgive me, but I don't see where the line you quoted exists. The description for this exercise reads as follows:

A matrix may have zero or more saddle points.

Your code should be able to provide the (possibly empty) list of all the
saddle points for any given matrix.

Can you please point me to the source of the line you quoted? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

@stkent It's in this repository's README. However, I think the empty list might after all be better in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, now I see, thanks! And yes, I think we now have three votes for an empty results list rather than an explicit null result :).

[3, 1, 2],
[2, 3, 1]
],
"expected": []
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@ErikSchierboom ErikSchierboom dismissed their stale review April 21, 2017 11:28

Not relevant anymore

@Insti Insti merged commit 73f4394 into master May 1, 2017
@Insti
Copy link
Contributor

Insti commented May 1, 2017

Thanks for your work on this @stkent

@Insti Insti deleted the saddle-points-canonical-data branch May 1, 2017 20:43
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