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

Update 10-defensive.md - Checkout #829

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from

Conversation

noise42
Copy link

@noise42 noise42 commented Jun 5, 2020

Added a more thorough explanation of the AssertionErrors thrown with the last implementation of range_overlap() and the right corrections that should be made (which covers three points: returning None, initializing from data, managing empty inputs)

Please delete this line and the text below before submitting your contribution.


Thanks for contributing! If this contribution is for instructor training, please send an email to checkout@carpentries.org with a link to this contribution so we can record your progress. You’ve completed your contribution step for instructor checkout just by submitting this contribution.

Please keep in mind that lesson maintainers are volunteers and it may be some time before they can respond to your contribution. Although not all contributions can be incorporated into the lesson materials, we appreciate your time and effort to improve the curriculum. If you have any questions about the lesson maintenance process or would like to volunteer your time as a contribution reviewer, please contact The Carpentries Team at team@carpentries.org.


Added a more thorough explanation of the AssertionErrors thrown with the last implementation of range_overlap() and the right corrections that should be made (which covers three points: returning None, initializing from data, managing empty inputs)
Copy link
Contributor

@ldko ldko left a comment

Hi @noise42, I think your addition here of a further explanation of the test failures and adding corrected code will be helpful for people. I have made some suggestions related to the following:

  • Keep line length under 100 characters
  • Use consistent spelling of "behavior"
  • Use back ticks consistently when referring to Python names
  • Format comments according to Pep8 style guidelines.
  • Place statements on separate lines
  • Re-run test_range_overlap and let learners know to expect no output indicates tests passing

If you agree with any of the changes, please go ahead and add them to the pull request. If you disagree with any or have questions, please feel free to let me know. Thank you very much for contributing to the project!

and if we trace the behavior of the function with that input,
we realize that we're initializing `max_left` and `min_right` to 0.0 and 1.0 respectively,
and if we trace the behavior of the function with that input, we realize that there is nothing in the code that could return None (which is our desired behaviour).
Moreover, not only the first assertion fails. If we try the other assertions we can see that the fourth line produces another AssertionError, and we must realize that we're initializing `max_left` and `min_right` to 0.0 and 1.0 respectively,
Copy link
Contributor

@ldko ldko Jun 12, 2020

Choose a reason for hiding this comment

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

I suggest tweaking this to:

and if we trace the behavior of the function with that input, we realize there is nothing in the
code that could return `None` (which is our desired behavior).
Moreover, not only the first assertion fails. If we try the other assertions, we see that the
fourth line produces another `AssertionError`, leading us to realize that we're initializing
`max_left` and `min_right` to 0.0 and 1.0 respectively,

regardless of the input values.
This violates another important rule of programming:
*always initialize from data*.
The last assertion would be thrown too, because we did not specify any behaviour on empty inputs.
Copy link
Contributor

@ldko ldko Jun 12, 2020

Choose a reason for hiding this comment

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

I suggest tweaking this to:

The last assertion also fails because we did not specify any behavior in the case of `ranges`
being empty.

regardless of the input values.
This violates another important rule of programming:
*always initialize from data*.
The last assertion would be thrown too, because we did not specify any behaviour on empty inputs.

The correct *range_overlap* function shall be therefore:
Copy link
Contributor

@ldko ldko Jun 12, 2020

Choose a reason for hiding this comment

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

Perhaps:

Let's correct `range_overlap`:

min_right = min(min_right, right)
#the returned values should change according to the final results: if the left end is greater than the right end then there is no overlap, thus returning None
return (max_left, min_right) if max_left < min_right else None
~~~
Copy link
Contributor

@ldko ldko Jun 12, 2020

Choose a reason for hiding this comment

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

I like that you are providing corrected code and that you added comments to help explain what is happening in the function. I suggest shortening the comments and keeping all of the statements on individual lines to avoid adding anything new that might confuse new learners:

def range_overlap(ranges):
    '''Return common overlap among a set of [left, right] ranges.'''
    # Set behavior for empty inputs.
    if not ranges:
        return None

    # Initialize our max and min using the first range.
    max_left = ranges[0][0]
    min_right = ranges[0][1]

    for (left, right) in ranges:
        max_left = max(max_left, left)
        min_right = min(min_right, right)

    if max_left < min_right:
        return (max_left, min_right)
    else:
        # No overlap was found.
        return None

I also wonder what you think about adding a re-running of the tests after this new code is given to let learners know to expect no output on successful tests:

Now running our test, we expect no output, indicating all of our assertions passed:
~~
test_range_overlap()
~~
{: .language-python}

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.

None yet

2 participants