Conversation
anjali-deshpande-hub
left a comment
There was a problem hiding this comment.
Good commenting with the PR! I liked that you tested all operations with patient_summary function and have added more test cases for detect_problems. I have a a few review comments and would like to request changes:
- Replace the hardcoded absolute paths to relative paths so that a Learning support can run the code successfully as well. For example: change
"/Users/innas/python1/python/05_src/data/assignment_2_data/inflammation_01.csv"to"../../05_src/data/assignment_2_data/inflammation_01.csv" - The call to readlines() method has to be read in a variable. Instead of
print(f.readlines()), read it into a variable, If you do that, you won't have to hardcode the total number of lines in the for loop (range(60)) that follows. - I like that you have tested when the operation is not recognized by the function (passing 'mod'). It would be nice to catch the error and die gracefully. You can modify the test case to using try and except block like this:
try:
patient_summary(all_paths[0], 'mod')
print("Test failed: No exception raised")
except ValueError as e:
print("Test passed: ", e)
|
@anjali-deshpande-hub thank you for your feedback! I have committed the changes. could you please take a look? |
anjali-deshpande-hub
left a comment
There was a problem hiding this comment.
Well done with the review changes!
Just one last review comment which I skipped in the first review.
Changes to assignment_1.ipynb should not be included in this pull request.
Please refer to following thread of slack https://uoft-dsi-certificates.slack.com/archives/C08JWD6RRU3/p1747084222526419 to fix this.
|
@anjali-deshpande-hub thank you! |
|
I think the reason assignment_1.ipynb is still showing as changed in this PR is because after you merged/closed PR #1, which merged the assignment_1 branch into main on GitHub, you didn’t update your local main branch. So when you ran git checkout main -- assignment_1.ipynb to revert the file in this PR, it used the outdated local main, not the updated one from GitHub. To fix this: Run Run Switch back to your feature/PR branch, and then re-run the commands from the Slack thread (like you did earlier) to properly revert assignment_1.ipynb. |
What changes are you trying to make? (e.g. Adding or removing code, refactoring existing code, adding reports)
I am adding code to both sections of the 2nd assignment.
What did you learn from the changes you have made?
I have learned to work with .csv files, analyze them and check errors. I also learned more about numpy.
Was there another approach you were thinking about making? If so, what approach(es) were you thinking of?
Not really.
Were there any challenges? If so, what issue(s) did you face? How did you overcome it?
Not many, was just playing with the reading operator for the .csv files
How were these changes tested?
Locally, then in the files which was submitted here
A reference to a related issue in your repository (if applicable)
N/A
Checklist