FermiQC / Fermi.jl

Fermi quantum chemistry program
MIT License
134 stars 24 forks source link

Non-concrete `Atom` types in `Molecule` #118

Open thchr opened 2 years ago

thchr commented 2 years ago

https://github.com/FermiQC/Fermi.jl/blob/721b55fe513cda2c79e5773bf950f00caeb66f89/src/Core/Molecule.jl#L64

I was browsing your repo and noticed that the Atom typed defined in Molecules.jl is parametric in {I,F}: but here, this parametric dependence is not accounted for. As a result, any access of the elements of the atoms field's elements will be type-unstable.

gustavojra commented 2 years ago

Hey! Thanks for pointing it out. I don't know how I missed your issue until now. We have dropped this parametric dependency in the Atom struct. Now it simply reads as:

struct Atom
    Z
    mass
    xyz
end

Found in: https://github.com/FermiQC/Molecules.jl/blob/6e32b19d16d8052fc613461ed60d61733fa3254c/src/Molecules.jl#L19

thchr commented 2 years ago

Hi @gustavojra. That change has some significant side-effects though: now, every access of a field of an Atom is type-unstable. Effectively, this will make anything that uses an Atom type-unstable. It seems the right fix is to keep Atom parametric and simply make any struct containing an Atom parametric as well.

gustavojra commented 2 years ago

Ok, I think I misunderstood your first comment. We removed the type declarations so that we could use Atom with AutoDiff routines, but now I see how that can take a toll on the overall performance. I am changing Atom and Molecule to

struct Atom{T<:Real, X<:Real}
    Z::Int
    mass::T
    xyz::SVector{3, X}
end

struct Molecule{A<:Atom}
    atoms::Vector{A}
    charge::Int
    multiplicity::Int
    Nα::Int
    Nβ::Int
end

Hopefully, that solves the problem. Unless T and X must appear as parameters for Molecule as well.