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

ENH: Remove all potential import circles by copying docstrings #541

Merged
merged 6 commits into from
Oct 18, 2023

Conversation

bifbof
Copy link
Collaborator

@bifbof bifbof commented Sep 26, 2023

This PR removes all imports in the class files that will later lead to circular imports.
In #540 I often used the function @doc for this, in this PR that isn't the case.
Why? Because removing the docstring from all functions makes the source code harder to read and the advantages that @doc gives can also be done easily with just adjusting the docstrings by hand. Also it was so much easier to do.
So what does this PR include?

  • @doc usage for simple to_csv and to_postgis functions/methods. Here the functions are so simple that the docstring is not necessary, plus all classes share the same docstring anyway.
  • Copying the docstring for all other methods/functions. Additionally, adding a comment at each docstring as a reminder which other docstrings should also be changed as well.
  • Adding argument names instead of *args, **kwargs. Argument names are also documentation and should not be avoided.
  • Small cleanups where certain things no longer work due to imports/changed argument names.

There is a lot of overlap in the documentation of certain methods, e.g. spatial_filter. We can move these documentations into the TrackintelGeoDataFrame and thus reduce the number of copies. However, this is not part of this PR, as this PR is already too large.

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (963fb9e) 93.43% compared to head (8de386e) 93.28%.
Report is 1 commits behind head on master.

❗ Current head 8de386e differs from pull request most recent head 54e7b2e. Consider uploading reports for the commit 54e7b2e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
- Coverage   93.43%   93.28%   -0.15%     
==========================================
  Files          32       32              
  Lines        2162     2114      -48     
  Branches      434      400      -34     
==========================================
- Hits         2020     1972      -48     
  Misses        129      129              
  Partials       13       13              
Files Coverage Δ
trackintel/__init__.py 100.00% <100.00%> (ø)
trackintel/analysis/labelling.py 96.77% <ø> (ø)
trackintel/analysis/modal_split.py 100.00% <ø> (ø)
trackintel/analysis/tracking_quality.py 100.00% <ø> (ø)
trackintel/geogr/distances.py 96.66% <ø> (-0.09%) ⬇️
trackintel/io/file.py 100.00% <100.00%> (ø)
trackintel/model/locations.py 100.00% <100.00%> (ø)
trackintel/model/positionfixes.py 100.00% <ø> (ø)
trackintel/model/staypoints.py 100.00% <100.00%> (ø)
trackintel/model/tours.py 100.00% <100.00%> (ø)
... and 8 more

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

@bifbof bifbof requested review from hongyeehh, NinaWie and henrymartin1 and removed request for hongyeehh and NinaWie October 14, 2023 10:43
Copy link
Member

@NinaWie NinaWie left a comment

Choose a reason for hiding this comment

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

Wow, huge change in the docstrings, thanks for the effort to clean this up and remove circular imports.
It seems that with the new version, it can lead to inconsistencies when someone adapts the docstring and forgets to change it in the other place (despite your comments). I understand your argument that it's important to have documentation in the source code, but I'm still unsure whether we should do that everywhere. In particular in the model/xx.py files it might not be so important to have the docstring for the function in the source code, because every method is just a one-liner calling the actual function. But I leave it up to Henry and Ye, no strong opinion on this.

trackintel/model/positionfixes.py Outdated Show resolved Hide resolved
@bifbof
Copy link
Collaborator Author

bifbof commented Oct 18, 2023

Okay updated the docstrings, made the minimal change to get this through.
We could also (in another PR) add this See also section to connect the functions back to the methods.
The big bulk of changes left is basically only removing the stuff that was done to make the class Positionfixes circle-free.
Ready for a second round of review @NinaWie

Copy link
Member

@NinaWie NinaWie left a comment

Choose a reason for hiding this comment

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

Looks good to me now :)

@bifbof bifbof merged commit a55e2e2 into mie-lab:master Oct 18, 2023
6 checks passed
@bifbof bifbof deleted the no_circles branch October 18, 2023 13:50
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