pointOfive / STA130_F23

Python/JupyterHub implementation of this UofT classic
10 stars 14 forks source link

Homework and Tutorial 7 #15

Closed mistryrohan closed 1 year ago

mistryrohan commented 1 year ago

This is the "tester" version of both, I will make the student-facing one after changes from the review. Ignore the "Test" and "Testing changes" commits, those were for the video and I do not want to make a whole new branch just to pull my homework and tutorial.

Also, the tutorial images probably will not work here, but they're in the resources for TUT7, I just do not want the link to be local and people having to download it.

mistryrohan commented 1 year ago

I can fix the conflict above once someone edits the homework and tutorial files.

pointOfive commented 1 year ago

This is shaping up to be a great homework; but, I have quite a bit of revision that I'd like executed to get it there!

mistryrohan commented 1 year ago

Should be good for a final small review if it is even needed. I added images to the 'im' folder in our GitHub (mimicking what Scott did for the syllabus so I hope it works).

lucieyang1 commented 1 year ago

Do you have the assignment on MarkUs and the student-facing file? For testing. I don't see it on my end right now.

mistryrohan commented 1 year ago

I have added them, I am not sure if they are working as intended though. It does not show any test output on whether it passed or failed on my end after I made some changes.

Edit: Nevermind they do seem to work it was just really slow for some reason

lucieyang1 commented 1 year ago

I see the MarkUs assignment now! The file in your fork seems to have solutions and the tests. Where can I find the student-facing file? Thanks :))

mistryrohan commented 1 year ago

I have to change that up since I had to do set seeds on the plots now so I should have that tomorrow before I review Mathew's new changes. Sorry about that.

lucieyang1 commented 1 year ago

Hello, I don't think the MarkUs tests are working for the updated files, but I did compare my answers to the tester.

Everything else looks good to me! :))

Edit: I have made all these edits directly to the files. I also changed the tester file so that students don't see the answer with MarkUs tests. Feel free to revert if you disagree with anything!! I also tried uploading the tester file on MarkUs and running tests, but they do not seem to be working. Not sure why.

pointOfive commented 1 year ago

Lucie and I ended up editing at the same time. Lucie's additions to the tester file won't conflict at all with anything I've done as I've not yet addressed the tester file.

For the student facing notebook, don't look at Lucie's edits until you've finalized my version updates (which was based on your last version). Then, click on Lucie's commits and see if something there could overwrite or improve what I've already provided in my commit.

pointOfive commented 1 year ago

Here are the notes I've made on my edits/updates commit:

pointOfive commented 1 year ago

Once you've given another pass based on my commits and then reviewed Lucie's commits, you'll finalize the tester file. It seem Lucie had some problems getting the tester file to work. Not sure, but that probably referred to student run tests since you thought they were working for you (probably for the instructor run tests). When you've got the final tester file all synchronized with the new way the homework is, see if you can get the tester file working. If not, let me know so I can take a look into what might be going wrong. These MarkUs tester files rarely seem to quite work right off the bat in my experience...

ps -- I've already said this, but Lucie's general comments/feedback about how submissions/testing needs to be specified just so are all exactly correct, so make sure the student facing file and tester file correctly address the issues Lucie's raised.

pointOfive commented 1 year ago

Tutorial commit incoming.

pointOfive commented 1 year ago

Not sure where the disconnect was (perhaps just overall time demand), but it doesn't look like the things I asked for were completed. (i.e., mostly, tester file has not been synched/finalized!) I've gone ahead and done what's needed this time. Here are the notable edits I needed to make to address my requests above: https://github.com/pointOfive/STA130_F23/pull/15#issuecomment-1603050532

student facing (STA130_HW_7_student.ipynb)

tester (STA130_HW_7.ipynb)

...

Oo... the tester file has not been updated to reflect the new indexing/enumerating of the student facing file... hmm...

Going to pause for a moment here and consider options. [Things are synched up to Q10/Q11 between STA130_HW_7.ipynb and STA130_HW_7_student.ipynb]

pointOfive commented 1 year ago

Ah -- I see on MarkUs STA130_HW_7_tester.ipynb from June 15: perhaps this is the updated file.

mistryrohan commented 1 year ago

Not sure where the disconnect was (perhaps just overall time demand), but it doesn't look like the things I asked for were completed. (i.e., mostly, tester file has not been synched/finalized!) I've gone ahead and done what's needed this time. Here are the notable edits I needed to make to address my requests above: #15 (comment)

student facing (STA130_HW_7_student.ipynb)

  • removed solutions from student file!!

tester (STA130_HW_7.ipynb)

  • updated to match student facing files so there's no versioning differences that might be confusing later
  • commented out broadway = broadway.drop('top_ticket_price', axis=1) since if this will thrown and error or make the answer automatically correct if run in the tester file!
  • exposed some answers like plots Q1/Q2 in an automated way assigning hint in preceding cell (so it doesn't print twice) and then formatting the output in the way I'd like this to be done in the future
  • Q3: hiding answer as test variable in preceding cell so it's not automatically exposed
  • Solutions won't show in MarkUs unless they're output as messages with assert statements! [which is what I meant by my request to review how "autofails" are done in Lucie's HW3]
  • synching versions and perhaps some small edits here and there
  • Q11 answer missing an answer in the tester file? failed to update, it looks like

...

Oo... the tester file has not been updated to reflect the new indexing/enumerating of the student facing file... hmm...

Going to pause for a moment here and consider options. [Things are synched up to Q10/Q11 between STA130_HW_7.ipynb and STA130_HW_7_student.ipynb]

I wanted to ask if you were looking at the correct version of the homework because the student file does not have any answers in it and Q11 does have an answer in the tester file. Also, I did do the assert False, hint thing as in Lucie's file.

pointOfive commented 1 year ago

Did not git pull locally after switching over to this branch: was not using the most up to date file(s). Will compare my made changes to the current updates: perhaps all is completed after all (which would be great!)

pointOfive commented 1 year ago

Yes -- exactly right @mistryrohan -- I was not using your up to date versions(!!!), but I have them now as follows:

mistryrohan commented 1 year ago

Fairly sure that was just a rename because I wanted to make it more explicit but MarkUs has multiple files with similar names on there right now.

pointOfive commented 1 year ago

Ah the world makes sense again :) very happy to see I was just looking at the wrong (old version) files. Excellent (and a good error to make so I'll be more carefully about this in the future! Always git pull to stay up with the times!).

This looks great. I'll confirm on MarkUs tomorrow; then, finalize the TUT; then, merge the PR and we're done!

pointOfive commented 1 year ago

Got everything working on MarkUs. Had some problems forgetting to put # test_Qx in tester cells which caused errors and subsequent tester cells to fail. Also had to move # test_Q18 cells up as the last cells of the tester file are not included in the merge after the last matching cell of the student file... Took a while to track this all down an figure out what was happening...