Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix functions notebook errors #251

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 15 commits into from
Nov 28, 2024
Merged

Conversation

edoardob90
Copy link
Member

@edoardob90 edoardob90 commented Oct 22, 2024

Fix #237

@edoardob90 edoardob90 linked an issue Oct 22, 2024 that may be closed by this pull request
@edoardob90 edoardob90 self-assigned this Oct 29, 2024
@edoardob90 edoardob90 added the upcoming tutorial Stuff to work on/fix before the next tutorial label Nov 8, 2024
@edoardob90 edoardob90 marked this pull request as ready for review November 26, 2024 12:09
Copy link
Collaborator

@Snowwpanda Snowwpanda left a comment

Choose a reason for hiding this comment

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

Had a look through the changes, the exercises are still too difficult in my opinion but your work definetly helps. I made some comments about older exercises too where I understand with increased difficulty we can expect more from the user, but still I would reorganise a bit, to avoid too much frustration.

functions.ipynb Show resolved Hide resolved
functions.ipynb Outdated Show resolved Hide resolved
functions.ipynb Outdated Show resolved Hide resolved
functions.ipynb Outdated
" for item in args[1:]:\n",
" result += item\n",
"\n",
" return result "
Copy link
Collaborator

@Snowwpanda Snowwpanda Nov 26, 2024

Choose a reason for hiding this comment

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

Most of the solution is already there? remove.

Make it clear that although the type of args can be any, they must all be the same.

Define what it means to sum up two None.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe even leave the (None, None) testcase out, not sure how helpful it is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At some places the ref. function doesn't use the same names for the input variables as the solution function, change to be consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exercises in the end are missing docstrings.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exercise longest_sequence:

Part 1 already tests part 2 as well, I assume this is because of the naming of the exercises? in anycase an n^2 solution doesn't pass part1

I don't like the solution at all. I would show the solution with sorting first, which runs fine and is much better understandable than the O(n) solution, saving a log factor shouldn't matter for them.
If you really want you can have two solutions for part1 and 2 which show a nice improvement by the use of sets and hashing.

For part 2 I get no feedback when run, i could imagine this is again an issue with the naming of the exercise?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exercise password_validator2:

Solution contains Counter function which is not explained.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Buckets reorginaziation:

Import necessary libraries (pathlib and ascii_lowercase, ascii_uppercase?) in the exercise cells.

Make the reference_solutions self contained. It should be possible to copy paste the ref solution and pass the tests. This also means the input variable names should be consistent

Copy link
Collaborator

@Snowwpanda Snowwpanda left a comment

Choose a reason for hiding this comment

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

I like ths split of exercise two, feels much more natural now. I still don't like the formatting, either change or explicitly tell them how to do it with a hind.
Also part 2 needs some change

functions.ipynb Show resolved Hide resolved
tutorial/tests/test_functions.py Outdated Show resolved Hide resolved
Snowwpanda
Snowwpanda previously approved these changes Nov 28, 2024
Copy link
Collaborator

@Snowwpanda Snowwpanda left a comment

Choose a reason for hiding this comment

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

This looks good, thanks

@edoardob90 edoardob90 merged commit a6a77f7 into main Nov 28, 2024
1 check passed
@edoardob90 edoardob90 deleted the fix/errors-functions-notebook branch November 28, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upcoming tutorial Stuff to work on/fix before the next tutorial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors found in the functions notebook
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.