Skip to content

954 modified Atoms.index#955

Merged
zerothi merged 8 commits intozerothi:mainfrom
tfrederiksen:954-atoms-index
Oct 1, 2025
Merged

954 modified Atoms.index#955
zerothi merged 8 commits intozerothi:mainfrom
tfrederiksen:954-atoms-index

Conversation

@tfrederiksen
Copy link
Copy Markdown
Contributor

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.91%. Comparing base (5be4413) to head (e1d040c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #955   +/-   ##
=======================================
  Coverage   86.91%   86.91%           
=======================================
  Files         412      412           
  Lines       54333    54349   +16     
=======================================
+ Hits        47221    47237   +16     
  Misses       7112     7112           

☔ 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.

Copy link
Copy Markdown
Owner

@zerothi zerothi left a comment

Choose a reason for hiding this comment

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

First, thanks!

I've thought about it, and the way it looks is somewhat not coherent with the other routines which only allows the list-like entry.

I can adjust the PR if you'd like? Then I'll ping you for comments? Thanks!

@tfrederiksen
Copy link
Copy Markdown
Contributor Author

I can adjust the PR if you'd like? Then I'll ping you for comments? Thanks!

Maybe I can first give it another try taking into account your comments?

By the way, I've always been a bit confused about species vs atom. What's the distinction?

@zerothi
Copy link
Copy Markdown
Owner

zerothi commented Sep 30, 2025

So the species is an internal reference to the unique species in the Atoms object. So if you have H(szp) and H(dzp) then you would have 2 species. However, the Atoms object will only have 2 unique Atom instances (one for each). And then the species index will be an array of #-of atoms, and each value will be 0 or 1, depending on which atomic species it is.

So internally, sisl finds which atom is corresponding to the requested one, and by that it has the species index, then it just does (atoms.species == species_index).nonzero()[0] and you have all the indices of that atom. Hope that makes sense?

Copy link
Copy Markdown
Owner

@zerothi zerothi left a comment

Choose a reason for hiding this comment

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

minor nit-picks... ;)

@tfrederiksen
Copy link
Copy Markdown
Contributor Author

Should we also add an example in the docstring how to use this?

@zerothi
Copy link
Copy Markdown
Owner

zerothi commented Oct 1, 2025

that would be great! :)

@zerothi
Copy link
Copy Markdown
Owner

zerothi commented Oct 1, 2025

@tfrederiksen do you know why it stopped? I think it's good, so I'll just merge, unless you object?

@tfrederiksen
Copy link
Copy Markdown
Contributor Author

@tfrederiksen do you know why it stopped? I think it's good, so I'll just merge, unless you object?

No, I don't know why. From my side it is ready.

@tfrederiksen
Copy link
Copy Markdown
Contributor Author

Hang on a sec...

@tfrederiksen
Copy link
Copy Markdown
Contributor Author

I think I found an issue, with multiple atoms one would end up with a list of arrays. I think with np.concatenate is should now be OK (now also tested with a separate script manipulating a geometry).

@zerothi zerothi merged commit be27d1e into zerothi:main Oct 1, 2025
4 of 15 checks passed
@zerothi
Copy link
Copy Markdown
Owner

zerothi commented Oct 1, 2025

Thanks!

@tfrederiksen tfrederiksen deleted the 954-atoms-index branch October 1, 2025 12:31
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.

Improve Atoms.index usability

2 participants