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

[WIP] Get DFTK fully generic in the floating point type #59

Merged
merged 16 commits into from Nov 18, 2019
Merged

Conversation

@mfherbst
Copy link
Member

mfherbst commented Nov 14, 2019

This PR will get DFTK agnostic to the floating point type used.

  • Get DoubleFloats to work
  • Tests
  • Merge magnesium examples
@mfherbst mfherbst added the feature label Nov 14, 2019
Copy link
Member

antoine-levitt left a comment

Looks great, victory is near! Left a few comments.

examples/magnesium_supercell.jl Outdated Show resolved Hide resolved
examples/magnesium_supercell.jl Outdated Show resolved Hide resolved
src/core/PlaneWaveBasis.jl Outdated Show resolved Hide resolved
src/core/PlaneWaveBasis.jl Outdated Show resolved Hide resolved
src/core/PlaneWaveBasis.jl Outdated Show resolved Hide resolved
src/core/energy_ewald.jl Outdated Show resolved Hide resolved
@@ -22,7 +23,7 @@ function energy_ewald(lattice, recip_lattice, charges, positions; η=nothing)
if η === nothing
# Balance between reciprocal summation and real-space summation
# with a slight bias towards reciprocal summation
η = sqrt(sqrt(1.69 * norm(recip_lattice ./ 2π) / norm(lattice))) / 2
η = T(sqrt(sqrt(1.69 * norm(recip_lattice ./ 2π) / norm(lattice))) / 2)

This comment has been minimized.

Copy link
@antoine-levitt

antoine-levitt Nov 14, 2019

Member

How do you more generally make sure that there are no implicit conversions to float64? Should it be T(1.69) and T(2pi) everywhere there are constant literals?

This comment has been minimized.

Copy link
@mfherbst

mfherbst Nov 16, 2019

Author Member

I do still not have a better solution here ... if you find one let me know.

src/core/generic_fftn.jl Outdated Show resolved Hide resolved
src/core/lobpcg_hyper_impl.jl Show resolved Hide resolved
src/core/self_consistent_field.jl Show resolved Hide resolved
@mfherbst mfherbst force-pushed the fully-generic branch from fedfef6 to eef5cb7 Nov 16, 2019
@mfherbst

This comment has been minimized.

Copy link
Member Author

mfherbst commented Nov 16, 2019

@antoine-levitt I'm waiting for your final verdict and then this goes in 😄.


n_electrons_integration = compute_n_elec(εF)
if abs(n_electrons_integration - n_electrons) > sqrt(eps(real(T)))
@warn("`find_occupation_gap_zero_temperature` has a mismatch in number " *

This comment has been minimized.

Copy link
@antoine-levitt

antoine-levitt Nov 16, 2019

Member

wrong copy-paste here in the error message. Shouldn't these be asserts? It really shouldn't happen...

This comment has been minimized.

Copy link
@mfherbst

mfherbst Nov 16, 2019

Author Member

Well but it does for some floating point types ... albeit the error is really neglible.

This comment has been minimized.

Copy link
@mfherbst

mfherbst Nov 16, 2019

Author Member

Ah no, scratch that ... that's at the other instance, here it can be a proper assert.

This comment has been minimized.

Copy link
@antoine-levitt

antoine-levitt Nov 16, 2019

Member

really ? The other one should really be fine also... that's in float16 I guess?

This comment has been minimized.

Copy link
@mfherbst

mfherbst Nov 16, 2019

Author Member

No, it's for more accurate types like Double32 or Double64. I think the reason is because their density of representable floats is plainly different than for IEEE floats, such that eps(T) might not always be the best metric, or put another way: The prefactor for an eps(T) error bound might just be different, not sure.

This comment has been minimized.

Copy link
@antoine-levitt

antoine-levitt Nov 16, 2019

Member

Yeah that's what I meant by "diagnostic tool". So I did a quick grep for backslash, things that might get us into trouble is pretty much all of PspHgh, SphericalHarmonics, bzmesh, A_coeff(n) = (-1)^n / (factorial(n) * 4^n * sqrt(π))

This comment has been minimized.

Copy link
@mfherbst

mfherbst Nov 16, 2019

Author Member

Ah I see ... puh ... I can't be bothered to fix that all here. I made a separate issue for that, see #60.

This comment has been minimized.

Copy link
@mfherbst

mfherbst Nov 16, 2019

Author Member

Hmm ... but some of that might not actually matter that much, because it's later assigned to arrays of the appropriate type.

This comment has been minimized.

Copy link
@antoine-levitt

antoine-levitt Nov 16, 2019

Member

yeah but you lose precision, which matters if we want to push below Float64. Let's do that later, but keep the assert (which is pretty useful precisely because of this kind of things; in fact a better check would be to check against eps^2/3 or something)

This comment has been minimized.

Copy link
@mfherbst

mfherbst Nov 16, 2019

Author Member

Ok, I dropped the relevant commit.

@@ -7,7 +7,7 @@ include("testcases.jl")
# TODO There is a lot of code duplication in this file ... once we have the ABINIT reference
# stuff in place, this should be refactored.

function run_silicon_redHF(;Ecut=5, test_tol=1e-6, n_ignored=0, grid_size=15, scf_tol=1e-6)
function run_silicon_redHF(T; Ecut=5, test_tol=1e-6, n_ignored=0, grid_size=15, scf_tol=1e-6)

This comment has been minimized.

Copy link
@antoine-levitt

antoine-levitt Nov 16, 2019

Member

T=Float64?

This comment has been minimized.

Copy link
@mfherbst

mfherbst Nov 16, 2019

Author Member

But then also at other places like in run_silicon_lda and so on ...
so far we explicitly specify the type everywhere, which I think is actually what you want.

@antoine-levitt

This comment has been minimized.

Copy link
Member

antoine-levitt commented Nov 16, 2019

Left of couple of nitpicking comments, but otherwise good to go!

@mfherbst mfherbst force-pushed the fully-generic branch from eef5cb7 to 1e94e8a Nov 16, 2019
@mfherbst mfherbst mentioned this pull request Nov 16, 2019
@mfherbst mfherbst force-pushed the fully-generic branch from 1e94e8a to 15ff911 Nov 16, 2019
@mfherbst mfherbst force-pushed the fully-generic branch from 15ff911 to e6b1bd4 Nov 16, 2019
@antoine-levitt antoine-levitt mentioned this pull request Nov 16, 2019
@mfherbst mfherbst force-pushed the fully-generic branch from 1906544 to 11dc169 Nov 16, 2019
@antoine-levitt

This comment has been minimized.

Copy link
Member

antoine-levitt commented Nov 17, 2019

As usual I'm thoroughly confused by the package system. I get

(v1.3) pkg> resolve
 Resolving package versions...
ERROR: Unsatisfiable requirements detected for package FourierTransforms [65ccaadd]:
 FourierTransforms [65ccaadd] log:
 ├─FourierTransforms [65ccaadd] has no known versions!
 └─restricted to versions * by DFTK [acf6eb54] — no versions left
   └─DFTK [acf6eb54] log:
     ├─possible versions are: 0.0.1 or uninstalled
     └─DFTK [acf6eb54] is fixed to version 0.0.1

on this branch

@mfherbst

This comment has been minimized.

Copy link
Member Author

mfherbst commented Nov 17, 2019

Ok ... I did some digging. Essentially all these problems boil down to the fact that FourierTransforms is unregistered and there seems to be no clean way to depend on unregistered packages from registered packages (as ours is now).

I propose the following: I'll open an issue asking about registration of the package (see 11 in FourierTransforms.jl). I would understand if that's not done to General, because it really is a little immature. In that case we have four options:

  • Do not allow for the generic stuff at all
  • Copy the files to DFTK
  • Register a fork in MolSim under a temporary name such that it is clear its a workaround
  • Register the package in MolSim as is. Requires some coordination with the authors to avoid registering different states under the same version or different versions for the same package state.

I personally prefer to the third option. It surely puts some maintenance burden on us, however.

What are your thoughts?

@antoine-levitt

This comment has been minimized.

Copy link
Member

antoine-levitt commented Nov 17, 2019

As usual I'm for the simplest and dirtiest solution :-p copy the files. It's fine under MIT (with proper attribution of course), it's just a temporary workaround to try stuff and it's the most flexible solution (eg if we want to change things in the FFT code)

@mfherbst mfherbst force-pushed the fully-generic branch from 11dc169 to 3d899a7 Nov 18, 2019
@mfherbst

This comment has been minimized.

Copy link
Member Author

mfherbst commented Nov 18, 2019

Hmm ... ok ... after some thoughts I agree with you. I closed the issue at FourierTransforms and added the files as a copy.

@mfherbst mfherbst force-pushed the fully-generic branch from 3d899a7 to d13cc8e Nov 18, 2019
@mfherbst mfherbst merged commit 8dab726 into master Nov 18, 2019
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-5.02%) to 79.054%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mfherbst mfherbst deleted the fully-generic branch Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.