search

worldbank / dec-python-course

16 stars 7 forks source link

project1-feedback #23

Open weilu opened 2 years ago

weilu commented 2 years ago

https://github.com/worldbank/dec-python-course/blob/8248dd7504a416e47e8d7d8713212e525799a074/1-foundations/project-1-schelling-model/project1-solutions.py#L21

In python variables named with all caps are constants. Since the dimension of the city changes with different input perhaps it's not most appropriate to define it as a constant?

kbjarkefur commented 2 years ago

https://github.com/worldbank/dec-python-course/blob/8248dd7504a416e47e8d7d8713212e525799a074/1-foundations/project-1-schelling-model/project1-solutions.py#L21

In python variables named with all caps are constants. Since the dimension of the city changes with different input perhaps it's not most appropriate to define it as a constant?

I noted that as well. I think we should update for later, but no need to do before session, as in this exact example grid size is not changing. But since it is a better practice I think we should change it for the future. And the name can be city_size instead of n as N/n should is rarely used for a dimension of a matrix, more so the total number of items in a matrix.

kbjarkefur commented 2 years ago

Make sure test_functions passes same type as the doc string says. I.e. lists and not tuples.

weilu commented 2 years ago

Consider putting the prep cell code in a single cell – easier to run

weilu commented 2 years ago

The enumerate example, consider changing the variable names to i, val as j is conventionally used as a name for index var

weilu commented 2 years ago

Tests for similarity function may not be sufficient. One participant's solution for similarity passed all the tests but failed the tests for hypothetical similarity. Turned out his solution for similarity was not fully correct

kbjarkefur commented 2 years ago

tell people to read the doc string properly to understand what is inputted and what is expected to be outputted

weilu commented 2 years ago

Feedback from a participant: probably good to start with an easier task, like finding blank/available slot in a city, with a small grid e.g. 2x2 to help ease the participants in

weilu commented 2 years ago

One participant emailed me asking about why some cells are excluded from the list_available's expectations/tests. Turned out they didn't realize for a cell to be available it has to be unoccupied (they were just checking it's above threshold). Perhaps highlight in the instruction about both conditions?