Skip to content

[Compat] NeighbourLists v0.6, EquivariantTensors v0.5 - #329

Draft
Alexsp32 wants to merge 4 commits into
mainfrom
nl-et-compat
Draft

Alexsp32 wants to merge 4 commits into
mainfrom
nl-et-compat

Conversation

@Alexsp32

@Alexsp32 Alexsp32 commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

ACEpotentials now works with EquivariantTensors 0.5.3 and NeighbourLists 0.6.2.

  • NTtransformST → DPTransform: EquivariantTensors renamed this, keeping the same constructor.
  • Added int_type = Int to PairList() calls in order to get same results as NL v0.5.

Due to other changes in ET, the Silicon BLR RMSE has increased slightly causing the CI test to fail.
Unsure how to continue here.

Depends on: (GitHub CI will fail before these are merged, and versions are registered)

@jameskermode

Copy link
Copy Markdown
Collaborator

Thanks, I'll take a look and see if I can track down where the rise comes from. If we can explain/understand it, we can fix it. I made a seperate PR ACEsuit/EquivariantTensors.jl#83 to upgrade the version of neighbour lists that ET uses, but has not been merged yet. Is that causing an issue? Would give a performance boost on GPU.

@jameskermode

Copy link
Copy Markdown
Collaborator

The new ET changes the basis, so this would constitute a breaking change to ACEpotentials and should be part of a new 0.11.x release series.

@jameskermode

Copy link
Copy Markdown
Collaborator

Some quick review points:

  • Main reason CI is failing is that it's asking for ET 0.5.3 which is not yet released, 0.5.2 is the latest published version.
  • After that it's OK to bump up the liq RMSE a little bit, I checked and its only a small increase (E is 4.20e-4 against a 4.0e-4 threshold, so could increase to 4.5e-5)
  • Code changes look fine
  • There is quite a lot of unnecessary whitespace change, e.g in calculators.jl. Can you mininise that?
  • As noted above, migrating to the new ET will lead to backwards incompatible changes - old models won't load and evaluate correctly. This needs to be v0.11.0 for this reason. load_model() should look in the saved Manifest and warn or error when the saved ET version is below 0.5.
  • Claude also said the following, which I don't fully understand so leave to you to consider:

CpuId = "=0.3.1" is added as a direct dependency with an exact pin but is
never used in src. That pin stops every downstream environment from
resolving any other CpuId version. A macOS ARM precompile workaround
belongs in the developer's local environment or upstream, not in the
package's compatibility bounds.

@jameskermode

Copy link
Copy Markdown
Collaborator

Ah, I see you noted that the need for a new ET release before this can be merged, and the other PRs to be merged. I'll leave that with @cortner.

@Alexsp32

Alexsp32 commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the quick comments!

I made a seperate PR ACEsuit/EquivariantTensors.jl#83 to upgrade the version of neighbour lists that ET uses, but has not been merged yet. Is that causing an issue? Would give a performance boost on GPU.

I only noticed your PR after I'd already made mine. We're doing the same things, except you also made sure GPU support works. It would make more sense to merge yours and none of the other PRs should be adversely affected.

CpuId = "=0.3.1" is added as a direct dependency with an exact pin but is
never used in src.

CpuId is at the end of a long dependency chain via LuxLib. It has recently had an update to v0.3.2, which causes precompilation to fail for itself and ACEpotentials on ARM processors (e.g. any recent MacBooks). The reason for this isn't immediately obvious, so including it as a dependency until an update is pushed made sense to me. (There's currently discussion over whether this will happen and how it should be done, so I'm assuming it will take a while.)

Alternatively, we could just mention it in release notes / changelog.

As noted above, migrating to the new ET will lead to backwards incompatible changes - old models won't load and evaluate correctly. This needs to be v0.11.0 for this reason. load_model() should look in the saved Manifest and warn or error when the saved ET version is below 0.5.

I hadn't thought about that yet, but I agree.

This branch has not been deployed

No deployments
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