Skip to content
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

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

Conversation

iglesias-cardinale
Copy link
Contributor

Linting

  • All parts of the code I added pass pylint. I also cleaned up preparation.py so that it scores a 10/10. there's a lot going on in test_preparation.py that I don't understand, so I cleaned it up a little bit but didn't mess with it too much since I don't want to delete something useful for somebody.

Contents

preparation.py

  • Added get_timeseries(path)

Takes input of path to json file. Returns pandas time series with the index being a datetime object and the data being the concentration of CO2 / methane. Assumes that the json file has the same format as mauna-loa-data/flask_monthly.json.

test_preparation.py

  • Added unit test to make sure the output timeseries has the same length (# of data points) as the input json file.

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!

@iglesias-cardinale iglesias-cardinale added enhancement New feature or request Data Anything related to data labels Nov 5, 2024
@iglesias-cardinale iglesias-cardinale added this to the Homework 5 milestone Nov 5, 2024
@haiderabbas007
Copy link
Contributor

I can review this

@haiderabbas007
Copy link
Contributor

haiderabbas007 commented Nov 6, 2024

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

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

Correct

  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

image

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

  2. 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
Copy link
Contributor

laserlab commented Nov 6, 2024

  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
Copy link
Contributor

laserlab commented Nov 6, 2024

@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
Copy link
Contributor

  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
Copy link
Contributor

@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
Copy link
Contributor Author

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

@iglesias-cardinale
Copy link
Contributor Author

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.

@SchrodingersStruggle SchrodingersStruggle merged commit 3ea580a into ubsuny:main Nov 6, 2024
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Data Anything related to data enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants