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

slight refactorings, docs, typos #298

Merged
merged 9 commits into from
Dec 10, 2024
Merged

slight refactorings, docs, typos #298

merged 9 commits into from
Dec 10, 2024

Conversation

mikapfl
Copy link
Member

@mikapfl mikapfl commented Nov 29, 2024

@JGuetschow it was easier for me to add my refactorings as patches instead of doing them inline in github, so I created a PR into your branch with what I would change in your branch before merging. Changes:

  • regenerated api docs (happens if you run "make docs" locally")
  • added LocalTrendsStrategy to CSG usage docs
  • added some docstrings, moved method docstrings from the class's docstring to the method's docstring
  • unified calculate_right_boundary_trend and calculate_left_boundary_trend into one function with case distinctions
  • refactored get_shifted_time_value. I first did it with python lists, which is a bit simpler, but using the underlying numpy arrays without converting is much faster, so I did that in the end.
  • corrected typos wherever I found them.

Copy link

codecov bot commented Nov 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.93%. Comparing base (2bcbcec) to head (46b3a26).
Report is 10 commits behind head on csg_filling.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@               Coverage Diff               @@
##           csg_filling     #298      +/-   ##
===============================================
- Coverage        96.94%   96.93%   -0.01%     
===============================================
  Files               54       54              
  Lines             5205     5194      -11     
===============================================
- Hits              5046     5035      -11     
  Misses             159      159              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JGuetschow
Copy link
Contributor

Thanks for adding docs. I should have said that I intentionally left them for after your review so I don't have to rewrite them in case of changes. Sorry, didn't intend to make you write the docs.

@@ -12,23 +14,16 @@ class Gap:

Attributes
----------
type :
type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that my pycharm setting for docstrings is not correct, that added the colons, I think

@@ -71,15 +68,6 @@ class FitParameters:
minimal number of points to calculate the trend. Default is 1, but if the degree
of the fit polynomial is higher than 1, the minimal number of data points
the degree of the fit polynomial

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we use docstrings in the methods and not a methods section. I'll try to remember in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, both works, but generally I like the docs to be as close to the code as possible.

@mikapfl
Copy link
Member Author

mikapfl commented Dec 10, 2024

@JGuetschow merge yourself if you are happy (will merge into your branch, not into main directly)

@JGuetschow JGuetschow merged commit 2251fe9 into csg_filling Dec 10, 2024
16 of 17 checks passed
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

Successfully merging this pull request may close these issues.

2 participants