Skip to content

changed Point -> Coord in neighbor lists #917

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

Merged
merged 2 commits into from
May 13, 2025
Merged

changed Point -> Coord in neighbor lists #917

merged 2 commits into from
May 13, 2025

Conversation

zerothi
Copy link
Owner

@zerothi zerothi commented May 13, 2025

I just stumpled on this, @pfebrer comments?

Copy link

codecov bot commented May 13, 2025

Codecov Report

Attention: Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.93%. Comparing base (1a74fd1) to head (569ca17).

Files with missing lines Patch % Lines
src/sisl/geom/_neighbors/tests/test_finder.py 0.00% 4 Missing ⚠️
src/sisl/geom/_neighbors/_neighborlists.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #917   +/-   ##
=======================================
  Coverage   86.93%   86.93%           
=======================================
  Files         408      408           
  Lines       53833    53836    +3     
=======================================
+ Hits        46802    46805    +3     
  Misses       7031     7031           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

atom: int
"""The atom for which the neighbors are stored."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you want it like this? It looks worse to me

Copy link
Owner Author

Choose a reason for hiding this comment

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

Agreed, but when building the docs, I get errors on missing attribute docs.... This is the PEP suggested way of adding docs-strings to attributes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this is very strange, last time I checked sphinx took the documentation from the previous line's comment if it started with #:. This is no longer true?

Copy link
Owner Author

Choose a reason for hiding this comment

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

well, all I can say is that I get lots of errors building the docs, and in particular these ones pop up. With this, it gets fixed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Perhaps #: only works for module attributes (constants), and not class attributes. It isn't fully clear to me either, because in constant.py I do #:, and I don't get errors there!

@pfebrer
Copy link
Contributor

pfebrer commented May 13, 2025

It is just a matter of preference, so if you prefer it like this it is fine for me. Personally I don't have a preference for one or the other, but I do think that changing names in the public API should not be done lightly. So if you think it really makes much more sense, then OK, but if not I would keep it as is.

@zerothi
Copy link
Owner Author

zerothi commented May 13, 2025

It is just a matter of preference, so if you prefer it like this it is fine for me. Personally I don't have a preference for one or the other, but I do think that changing names in the public API should not be done lightly. So if you think it really makes much more sense, then OK, but if not I would keep it as is.

Thats why I would do it rather soonish, it hasn't been picked up yet. So better now than in some months/years.

Signed-off-by: Nick Papior <[email protected]>
@zerothi zerothi merged commit a4945e4 into main May 13, 2025
@zerothi zerothi deleted the point-coord branch May 13, 2025 11:32
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