Skip to content

Nexus: Rework structure.Structure.locate()#5958

Open
brockdyer03 wants to merge 11 commits into
QMCPACK:developfrom
brockdyer03:rework-structure-locate
Open

Nexus: Rework structure.Structure.locate()#5958
brockdyer03 wants to merge 11 commits into
QMCPACK:developfrom
brockdyer03:rework-structure-locate

Conversation

@brockdyer03

@brockdyer03 brockdyer03 commented May 11, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

This splits the generally overloaded function Structure.locate() into separate functions which each mimic prior behavior. It additionally provides a more convenient interface, as the previous function had code which (I believe) was inaccessible.

I've also added a test for the split that works both with the previous function and the new function.

What type(s) of changes does this code introduce?

  • Refactoring (no functional changes, no api changes)
  • Testing changes (e.g. new unit/integration/performance tests)

Does this introduce a breaking change?

  • Maybe

What systems has this change been tested on?

Laptop, Fedora 43 KDE Plasma
Python 3.14.4
uv 0.11.8
Numpy 2.4.4
Pytest 9.0.3

Checklist

    • I have read the pull request guidance and develop docs
    • This PR is up to date with the current state of 'develop'
    • This PR adds tests to cover any new code, or to catch a bug that is being fixed
    • Documentation has been added (if appropriate)

@brockdyer03 brockdyer03 requested a review from jtkrogel May 11, 2026 22:11
@brockdyer03 brockdyer03 self-assigned this May 11, 2026
@brockdyer03 brockdyer03 added nexus python Pull requests that update python code labels May 11, 2026
jtkrogel
jtkrogel requested changes May 19, 2026
View reviewed changes

@jtkrogel jtkrogel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Many :-(

Comment thread nexus/nexus/tests/test_structure.py
Comment thread nexus/nexus/structure.py
Comment thread nexus/nexus/structure.py Outdated
Comment thread nexus/nexus/structure.py Outdated
Comment thread nexus/nexus/structure.py Outdated
Comment thread nexus/nexus/structure.py Outdated
Comment thread nexus/nexus/structure.py Outdated
Comment thread nexus/nexus/structure.py
Comment thread nexus/nexus/structure.py Outdated
Comment thread nexus/nexus/tests/test_structure.py
assert(testing.check_object_eq(res,ref,atol=1e-12))
#end def test_rmg_transform


@jtkrogel jtkrogel May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Needs tests for other cases. Some locate_ functions have bugs that would have been caught.

jtkrogel
jtkrogel requested changes May 19, 2026
View reviewed changes

@jtkrogel jtkrogel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(again)

Comment thread nexus/nexus/structure.py Outdated
@brockdyer03

brockdyer03 commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

Just a note on that latest commit.

It looks like np.bool used to be an alias of Python's builtin bool, but that was changed/removed at some point. The NumPy type was, for a time, np.bool_, but after a while it got changed to np.bool (no trailing underscore). I think until we hit requiring NumPy >= 2.0 we should probably stick to np.bool_ since it looks like that will be retained for a while.

It's sort of hard to tell why only some of the tests with old NumPy failed, since the mix of versions and fails is a bit unusual. Here are the versions:

  • Ubuntu 22.04 LTS
    • NumPy 1.21.5
    • No failures
  • Ubuntu 24.04 LTS
    • NumPy 1.26.4
    • Failed (coverage builds)
  • Nexus Workflow (Python 3.14)
    • NumPy 2.4.4
    • No failures
  • Nexus Workflow (Python 3.10)
    • NumPy 1.22.0
    • No failures

If anyone stumbles across a problem with NumPy and bool aliases and backwards compatibility, hopefully they find this PR.

jtkrogel
jtkrogel requested changes May 22, 2026
View reviewed changes
Comment thread nexus/nexus/structure.py Outdated
self,
indices: int | list[int],
invert : bool = False,
) -> npt.NDArray[np.int64]:

@jtkrogel jtkrogel May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fix indentation

Comment thread nexus/nexus/structure.py Outdated
indices = np.asarray(indices, dtype=np.int64)

if invert:
mask = np.ones(self.size(), dtype=np.bool_)

@jtkrogel jtkrogel May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use dtype=bool will work regardless of whether numpy devs change their minds again about type names

Comment thread nexus/nexus/structure.py Outdated
mask[indices] = False
indices = np.arange(self.size())[mask]

indices = np.asarray(indices, dtype=np.int64)

@jtkrogel jtkrogel May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use int

Comment thread nexus/nexus/structure.py Outdated
self,
elements: str | int | Elements | list[str | Elements | int],
invert : bool = False,
) -> npt.NDArray[np.int64]:

@jtkrogel jtkrogel May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indentation

Comment thread nexus/nexus/structure.py Outdated

def locate_by_elements(
self,
elements: str | int | Elements | list[str | Elements | int],

@jtkrogel jtkrogel May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use TypeAlias

Comment thread nexus/nexus/structure.py Outdated
else:
cell = np.asarray(cell, dtype=np.float64)
if cell.shape != (self.dim, self.dim):
raise ValueError(

@jtkrogel jtkrogel May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

self.error

Comment thread nexus/nexus/structure.py Outdated
for i in range(len(self.elem)):
if self.elem[i]==atom:
else:
cell = np.asarray(cell, dtype=np.float64)

@jtkrogel jtkrogel May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use float

Comment thread nexus/nexus/structure.py Outdated

@jtkrogel jtkrogel May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Put on one line or a couple of lines, not dangling. This must have gotten through previously.

Comment thread nexus/nexus/tests/test_structure.py Outdated
[6.00000, 0.00000, 0.00000],
[0.00000, 6.00000, 0.00000],
[0.00000, 0.00000, 6.00000],
], dtype=np.float64),

@jtkrogel jtkrogel May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

indent, use float

Comment thread nexus/nexus/tests/test_structure.py
[ 0.690245, 1.335874, 2.186592],
[ 0.711045, 1.361274, 3.966292],
[-2.558655, 2.774774, 3.094192],
], dtype=np.float64),

@jtkrogel jtkrogel May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

indent, use float

@jtkrogel

jtkrogel commented May 22, 2026

Copy link
Copy Markdown
Contributor

In short: fix indentation and use native types in arrays. Use TypeAlias for long hints.

@brockdyer03 brockdyer03 requested a review from jtkrogel June 26, 2026 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jtkrogel jtkrogel Awaiting requested review from jtkrogel

Requested changes must be addressed to merge this pull request.

Assignees

@brockdyer03 brockdyer03

Labels

nexus python Pull requests that update python code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@brockdyer03 @jtkrogel