Skip to content

Update interface to use NQCBase.Structure type. #4

Merged
Alexsp32 merged 5 commits intomainfrom
structure-type
Feb 23, 2026
Merged

Update interface to use NQCBase.Structure type. #4
Alexsp32 merged 5 commits intomainfrom
structure-type

Conversation

@Alexsp32
Copy link
Member

@Alexsp32 Alexsp32 commented Nov 27, 2025

If we add a Structure type to NQCBase PR, the ASE interface should return converted structures in this type, and support direct conversion as well as conversion from individual atoms, positions and cells.

This PR makes breaking changes compared to previous versions - reflect
this in version changes

  • Update NQCD Docs in NQCDynamics.jl.
  • Make sure unit tests work.

@Alexsp32
Copy link
Member Author

CI tests should now pass, I've bumped the minimum NQCBase version to fit with changes here.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the ASE interface to align with the new NQCBase.Structure return type, and adjusts Python module loading to a runtime __init__ pattern.

Changes:

  • Change convert_from_ase_atoms to return NQCBase.Structure instead of a tuple.
  • Update ASE Python module import to be initialized in __init__() and accessed via a Ref.
  • Update tests and tighten NQCBase compat (and bump package version).

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/NQCDInterfASE.jl Switches conversion output to NQCBase.Structure; updates ASE import initialization.
test/runtests.jl Updates the test entrypoint to use the package module name.
test/ase.jl Updates assertions to validate NQCBase.Structure fields, adds multi-frame type checks.
Project.toml Updates version and NQCBase compat constraint.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 19 to 35
@@ -31,9 +35,9 @@ function convert_to_ase_atoms(atoms::Atoms, R::Vector{<:Matrix}, cell::AbstractC
end
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

The PR description mentions supporting direct conversion using the new NQCBase.Structure type, but convert_to_ase_atoms currently only accepts (atoms, positions, cell) inputs. If the intent is to allow convert_to_ase_atoms(::NQCBase.Structure), add an overload that forwards to the existing methods (and update tests/docs accordingly), or adjust the PR description if that support is out of scope.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@Alexsp32 Alexsp32 Feb 10, 2026

Choose a reason for hiding this comment

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

This is actually a good suggestion, but since Structure.info doesn't enforce specific types, the conversion of info contents into a good pythonic representation should be up to the user.

@Alexsp32 Alexsp32 merged commit fde1305 into main Feb 23, 2026
2 of 3 checks passed
@Alexsp32 Alexsp32 deleted the structure-type branch February 23, 2026 13:10
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