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

Remove or clarify commented-out code #7

Closed
3 tasks done
rmsare opened this issue Jun 12, 2019 · 1 comment
Closed
3 tasks done

Remove or clarify commented-out code #7

rmsare opened this issue Jun 12, 2019 · 1 comment

Comments

@rmsare
Copy link

rmsare commented Jun 12, 2019

It's nice to see the tests pass, but there are a few instances of commented-out code in your tests and the example notebooks. You should remove these if they aren't necessary, or clarify if they can be run in certain cases (like you do in ARIMA.py and elsewhere).

  • Remove old code and print statements in ARIMA.py: L45-46, L120, L155-160, L204-205, 210, 235, 237
  • Remove old code from CometTS.py. There are many in the plot functions like L139, L291-296, L357 and 375, L476-481, as well as old testing calls like the NDVI plot at L847.
  • Remove old code from tests. It looks like you rewrote the code to use pd.testing instead of standard assertions, but many commented-out assertions remain (e.g. line 62-65 and 67 of Comet_test.py). There's also a block-commented code snippet at the end of file that should be deleted.

This will make the examples more accessible and make it easier to extend or contribute to CometTS without wading through scratch code.

(You also might consider removing the statement "No plotting testing included, do your own damn plotting if you have a problem." I get that it's unrealistic to test all possible uses of bokeh and matplotlib in the CometTS workflow, but this could be a little off-putting to someone who might want to contribute.)

(JOSS review thread)

@rmsare
Copy link
Author

rmsare commented Jul 4, 2019

Looks good - code has been cleaned up substantially

@rmsare rmsare closed this as completed Jul 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant