Skip to content

pythagorean-triplet: Add missing constraints to description #1365

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 1 commit into from
Oct 15, 2018

Conversation

TheBestJohn
Copy link
Contributor

Added missing constraint to the Pythagorean triplet problem that is outlined in the linked Project Euler problem.

Added missing constraint to the Pythagorean triplet problem that is outlined in the linked Project Euler problem.
@TheBestJohn TheBestJohn changed the title Add missing constraints pythagorean-triplet: Add missing constraints Oct 11, 2018
@TheBestJohn TheBestJohn changed the title pythagorean-triplet: Add missing constraints pythagorean-triplet: Add missing constraints to description Oct 11, 2018
Copy link
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for contributing this, @TheBestJohn!

I have performed ad-hoc examination of canonical-data.json which shows that all triplets in the canonical data correctly conform to this rule, so no updates should be required there:

Thu Oct 11 18:57:57 DST 2018
Python 3.7.0 (default, Aug  6 2018, 20:07:46)
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import json
>>> with open('canonical-data.json') as cd:
...     data = json.load(cd)
...
>>> triplets = []
>>> for case in data['cases']:
...     triplets.extend(case['expected'])
...
>>> for (a, b, c) in triplets:
...     if a >= b or b >= c:
...             print(f"malformed: ({a}, {b}, {c})")
...
>>>

@coriolinus
Copy link
Member

I now plan to wait until Monday the 15th. This is to give other maintainers the chance to look at this and make comments and request changes if desired. If there are no objections, I plan to merge this on that date. I do not object if another maintainer wants to merge this before then.

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 for taking the time to improve this exercise @TheBestJohn. Also, thanks @coriolinus for the thorough review and look into the canonical data. 👍

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.

It is interesting because for the problem as currently stated in the description, finding the product a * b * c, there is no need to impose an ordering between these three. However, for the problem posed in the canonical data, the ordering does matter. (the fact that the description and canonical data are not in agreement is noted in #1211 currently)

So, since we will eventually want this a < b < c, there's no problem with having it now.

@TheBestJohn
Copy link
Contributor Author

TheBestJohn commented Oct 12, 2018

I now plan to wait until Monday the 15th. This is to give other maintainers the chance to look at this and make comments and request changes if desired. If there are no objections, I plan to merge this on that date. I do not object if another maintainer wants to merge this before then.

Ah yes I should have stated that I looked into the data and found no conflicts when submitting the pull request.

also @rpottsoh no problem :) I just figured that it would definitely affect the performance of generated answers. Every result I bruteforce where a>b, or b>c is a calculation that didn't need to be done.

As @petertseng says a*b*c order does not matter however if we were tasked with identifying a b and c then a and b would have 2 answers as currently stated.

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