ubsuny / CP1-24-HW5

Homework 5 template for CP1
1 stars 17 forks source link

Added get_timeseries to preparation.py and unit test to test_preparation.py #35

Closed iglesias-cardinale closed 3 weeks ago

iglesias-cardinale commented 3 weeks ago

Linting

Contents

preparation.py

Reviewer

This is the first time I've changed a file that somebody else created, but I think it all works. I'm doing my PR a little early in the hopes that I can fix any problems before class tomorrow. Let me know what I can improve, thanks!

haiderabbas007 commented 3 weeks ago

I can review this

haiderabbas007 commented 3 weeks ago

Here are a few comments:

1) The docstring for get_timeseries(path) is comprehensive and provides a good overview of the function's purpose and output. However, it can be slightly misleading since the plotting code is not part of the function itself. You might want to remove the plotting example from the docstring or clearly indicate that the plotting part is for demonstration and is not included in the function.

2) Indexing was creating an error. Error1

3) After fixing indexing, there was an AttributeError.

Error2

4) After fixing both, the code worked and a plot was produced.

Correct

5) However, the plot has a flat portion from 1971 to 1975. Upon inspecting the raw file, I found that the data for these 5 years was missing. It would be appropriate to start from 1976 I think? @laserlab

image

6) The module-level docstring mentions fft, inverse fft, and calculate frequencies functions but none of these are in the code.

7) For your test_preparation.py file, The functions calc_freq, inv_fft, and fft are referenced in comments, but they are not defined in either the test script or preparation.py.

laserlab commented 3 weeks ago
  1. However, the plot has a flat portion from 1971 to 1975. Upon inspecting the raw file, I found that the data for these 5 years was missing. It would be appropriate to start from 1976 I think? @laserlab

@haiderabbas007 Yes exactly, since this is your data collection task please open a new PR with the corrected data file

laserlab commented 3 weeks ago

@haiderabbas007 good review, but please use the review function and approve so it can get merged and you can get homework points. @SchrodingersStruggle Otherwise just merge it so we can go on

haiderabbas007 commented 3 weeks ago
  1. However, the plot has a flat portion from 1971 to 1975. Upon inspecting the raw file, I found that the data for these 5 years was missing. It would be appropriate to start from 1976 I think? @laserlab

@haiderabbas007 Yes exactly, since this is your data collection task please open a new PR with the corrected data file

Done

haiderabbas007 commented 3 weeks ago

@haiderabbas007 good review, but please use the review function and approve so it can get merged and you can get homework points. @SchrodingersStruggle Otherwise just merge it so we can go on

Done

iglesias-cardinale commented 3 weeks ago

Thank you for the review. I'll make the changes you mentioned.

iglesias-cardinale commented 3 weeks ago

I was unsure how to go about documenting the potential use of the function, so I added the example in the docstring. I agree that it could be confusing, but before looking into this I wasn't familiar with plotting using pandas so I thought the example might be useful to have. I'll remove it before class today.