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

enable sirius #34

Merged
merged 11 commits into from
Jun 2, 2021
Merged

enable sirius #34

merged 11 commits into from
Jun 2, 2021

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Apr 27, 2021

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@ltalirz
Copy link
Member Author

ltalirz commented Apr 27, 2021

@conda-forge-admin, please rerender

@github-actions
Copy link

Hi! This is the friendly automated conda-forge-webservice.
I tried to rerender for you, but it looks like there was nothing to do.

@ltalirz
Copy link
Member Author

ltalirz commented Apr 27, 2021

@AdhocMan cp2k also has a serial (openmp-enabled) build.
if we want the serial cp2k version also to be linked against sirius, I guess we should have a serial sirius version.
what do you think?

for the moment we can add sirius only to the parallel cp2k build

@ltalirz
Copy link
Member Author

ltalirz commented Apr 27, 2021

In any case, we'll still need to wait until the cp2k 8.2 release since cp2k 8.1 only supports libxc 4.3.x

@ltalirz ltalirz changed the title enable sirius [on hold] enable sirius Apr 27, 2021
@ltalirz ltalirz mentioned this pull request Apr 27, 2021
8 tasks
@AdhocMan
Copy link

@AdhocMan cp2k also has a serial (openmp-enabled) build.
if we want the serial cp2k version also to be linked against sirius, I guess we should have a serial sirius version.
what do you think?

for the moment we can add sirius only to the parallel cp2k build

Sirius always requires MPI, so I think it's best to leave it disabled for the serial version.

@ltalirz ltalirz changed the title [on hold] enable sirius enable sirius May 28, 2021
@ltalirz
Copy link
Member Author

ltalirz commented May 30, 2021

@AdhocMan Could you please let me know how to properly link against sirius?

I've tried added the sirius includes and am linking against the library but it complains about undefined symbols

@AdhocMan
Copy link

It looks like the changes required for CP2K to use the latest SIRIUS version did not make into the 8.2 release. Since conda does not seem to support multiple versions very well, I'd suggest applying a patch to CP2K. Here is a patch file that should work: https://gist.github.com/AdhocMan/bdf3f1065af4b5ff7aad0b00f875179f

@ltalirz
Copy link
Member Author

ltalirz commented May 31, 2021

Thanks for that patch!

Since conda does not seem to support multiple versions very well,

Just to understand, you are saying this could be handled on the conda side? What feature is missing here?

@AdhocMan
Copy link

AdhocMan commented May 31, 2021

Just to understand, you are saying this could be handled on the conda side? What feature is missing here?

An older version of SIRIUS should work. I just had the impression, that conda usually only has one version of a package. Looking into it now, it can be solved by creating an additional branch in the sirius feedstock for the required version.

@AdhocMan
Copy link

I'm not sure why the patch isn't working, maybe @mtaillefumier could help here. Alternatively, we could try it with SIRIUS version 7.0.2.

@mtaillefumier
Copy link

I think the patch might be faulty.

@mtaillefumier
Copy link

the previous patch was referring to something related to dbcsr, so I had to manually modify the patch. This new patch works on the latest master but should work on V8.2 as well

sirius.patch.txt

@mtaillefumier
Copy link

that's really strange since the patch removes exactly these two functions calls. I wonder if the pipeline apply the patch or not.

recipe/meta.yaml Outdated Show resolved Hide resolved
@ltalirz
Copy link
Member Author

ltalirz commented Jun 1, 2021

Oops... thanks for pointing out this oversight ;-)

@ltalirz
Copy link
Member Author

ltalirz commented Jun 2, 2021

psmp build

Number of FAILED  tests 1
Number of WRONG   tests 4
Number of CORRECT tests 2963
Total number of   tests 2968
GREPME 1 4 2963 0 2968 X

It seems that 4 of the sirius tests yield results outside the tolerance (some of them by more than an order of magnitude).

One of the tests failed:

$SRC_DIR/regtesting/Linux-x86-64-conda/psmp/TEST-Linux-x86-64-conda-psmp-2021-06-01_11-52-50/SIRIUS/regtest-1/He-full-potential.inp.out
+------------------------------+

Linearization energies
Atom : He, class id : 0
augmented waves
n =  1   l =  0   order = 0   enu =    -1.564330  +
n =  1   l =  0   order = 1   enu =    -1.564330  +
local orbitals
n =  1   l =  0   order = 0   enu =    -1.564330  +
n =  1   l =  0   order = 1   enu =    -1.564330  +

[get_singular_components] smallest eigen-value of the singular components:   0.0012990509462977
[diag_full_potential_first_variation_davidson] iterative solver tolerance:     0.000050000000
[diag_full_potential_first_variation_davidson] step: 0, current subspace size: 61, maximum subspace size: 341
[diag_full_potential_first_variation_davidson] step: 1, current subspace size: 101, maximum subspace size: 341
[diag_full_potential_first_variation_davidson] step: 2, current subspace size: 141, maximum subspace size: 341
[diag_full_potential_first_variation_davidson] step: 3, current subspace size: 181, maximum subspace size: 341
[diag_full_potential_first_variation_davidson] step: 4, current subspace size: 221, maximum subspace size: 341
[diag_full_potential_first_variation_davidson] step: 5, current subspace size: 261, maximum subspace size: 341
[diag_full_potential_first_variation_davidson] step: 6, current subspace size: 301, maximum subspace size: 341
[diag_full_potential_first_variation_davidson] step: 7, current subspace size: 341, maximum subspace size: 341
[solve] Lowest band energies
ik :  0,   -45.610114  -45.607113  -45.600415  -45.564806  -44.589979  -44.565682  -44.491899  -43.994386  -43.967995  -43.879579
terminate called after throwing an instance of 'std::runtime_error'
terminate called after throwing an instance of 'std::runtime_error'
  what():    what():  
=== Fatal error at line 243 of file /home/conda/feedstock_root/build_artifacts/sirius_1621525035642/work/src/k_point/k_point_set.cpp ===
search of band occupancies failed after 10000 steps


=== Fatal error at line 243 of file /home/conda/feedstock_root/build_artifacts/sirius_1621525035642/work/src/k_point/k_point_set.cpp ===
search of band occupancies failed after 10000 steps

Any comments @AdhocMan ?

@mtaillefumier
Copy link

sirius.patch.txt
this should fix the problem hopefully.

@mtaillefumier
Copy link

You can stop the pipeline if possible because it will fail. the Fe-DOS.inp has a wrong value for one of the parameters (I deleted it then added it again in my patch forgetting about changing the value of the parameter).
sirius.patch.txt

@ltalirz
Copy link
Member Author

ltalirz commented Jun 2, 2021

Thanks! I also invited you as a collaborator to my fork - in case you notice any further issues, feel free to push directly to the branch.

@mtaillefumier
Copy link

I will need to modify the patch again later but I need to test the changes before. But this should not be blocking you. If you do not mind I prefer to put the patch in the comments section to avoid screwing up your branch. And it is better for review as well

@ltalirz
Copy link
Member Author

ltalirz commented Jun 2, 2021

thanks, indeed all the tests pass now

Number of FAILED  tests 0
Number of WRONG   tests 0
Number of CORRECT tests 2968
Total number of   tests 2968
GREPME 0 0 2968 0 2968 X

@mtaillefumier Should this be merged?

@mtaillefumier
Copy link

Go ahead. It needs more work of SIRIUS side and it is not a limiting factor either.

@ltalirz ltalirz merged commit 9e995f1 into conda-forge:master Jun 2, 2021
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.

4 participants