Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatiblity with Unitful ? #56

Open
Joh11 opened this issue Apr 23, 2024 · 2 comments
Open

Compatiblity with Unitful ? #56

Joh11 opened this issue Apr 23, 2024 · 2 comments

Comments

@Joh11
Copy link

Joh11 commented Apr 23, 2024

Hi,

As we can see with this minimal example, reciprocalbasis does not work well with Unitful quantities:

using Unitful
using Bravais: reciprocalbasis

Rs = ([1.0, 0.0, 0.0], [-0.5, sqrt(3)/2, 0.0],   [0, 0, 1.25])
Us = map(Rs) do v v * 1u"" end

@show reciprocalbasis(Rs) # works
@show reciprocalbasis(Us) # StackOverflowError

It yields a stack overflow error. For me, this is likely due to the fact that the input and output dimensions are different ($L^3$ and $L^{-3}$ respectively), so type inference has some troubles.

Is there a possible workaround ? (apart from the trivial one of stripping the units before and putting them back after)

@thchr
Copy link
Owner

thchr commented Apr 23, 2024

Hmm, the implementation hits a recursive loop because I didn't account for the possibility of the eltypes of the input not being subtypes of Real.

The problematic bits are here:

reciprocalbasis(Rs) = reciprocalbasis(Tuple(Rs)) # type-unstable convenience accesor

function reciprocalbasis(Rs::Union{NTuple{D, <:AbstractVector{<:Real}},
StaticVector{D, <:AbstractVector{<:Real}}}) where D

The question, I suppose, is whether DirectBasis and ReciprocalBasis optionally should take a eltype which isn't Float64? Feedback and usecases would be welcome!

@thchr
Copy link
Owner

thchr commented Apr 23, 2024

So, just to be more specific, the issue here is that

T = eltype(eltype(Us)) # Quantity{Float64, 𝐋, Unitful.FreeUnits{(Å,), 𝐋, nothing}}
T <: Real # false

The current implementation assumes all vectors have elements that are subtypes of Real (and Quantity is not).

Related: PainterQubits/Unitful.jl#680

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

No branches or pull requests

2 participants