lsmo-epfl / aiida-lsmo

AiiDA workflows for the LSMO laboratory at EPFL
Other
9 stars 13 forks source link

Cp2kMultistageDdecWorkChain doesn't extract motion_step_info of MD stage #88

Closed mpougin closed 3 years ago

mpougin commented 3 years ago

I was trying to run the optimisation workchain with an additional MD stage at 400K. I tried different cp2k protocols ( 1) standard AiiDA protocol, 2) latest_multistage_curated.yaml, that worked fine for me for a 3 stages optimisation in the nanoporous screening workchain last year, 3) own settings with different STEPS and TIMESTEP settings), and different MOFs. I always receive a list index out of range error for output_dict['stage_info']['nsteps'].append(kwarg['motion_step_info']['step'][-1]) (see attached image1). Apparently, stage 1 (MD stage) doesn't produce any motion_step_info in the output dictionary (see attached image2). I checked the calculation on helvetios, the aiida.out file looks fine to me, no errors, SCF converged, all steps computed (see e.g. /scratch/pougin/aiida_run/d0/18/65e9-8dce-49d9-85ef-a48d294cc027). Stage_0 and Stage_2 (CELL_OPT stages) work fine and produce all required outputs. image1 image2

@ltalirz or @danieleongari can you help me with that? I don't manage to figure this out on my own.

ltalirz commented 3 years ago

@mpougin not sure but it might be an issue of the aiida-cp2k parser I see a somewhat worrying note here https://github.com/aiidateam/aiida-cp2k/blob/ffe2943f287e3c22488f04847efb7aa016ce9ba7/aiida_cp2k/utils/parser.py#L144

parsing seems to happen here https://github.com/aiidateam/aiida-cp2k/blob/develop/aiida_cp2k/utils/parser.py#L262

if you have the calculation node, you can reparse it directly from the verdi shell

i.e. put a few print statements / checks into the parser in aiida-cp2k create an instance of the parser class and then use this method to "re-parse" the calculation

https://aiida.readthedocs.io/projects/aiida-core/en/latest/reference/apidoc/aiida.parsers.html?highlight=parse_from#aiida.parsers.Parser.parse_from_node

danieleongari commented 3 years ago

The parser is incompatible with CP2K 8.1: so far e have been using 6.1.

From a super quick check I see that the steps are not printed because CELL ANGLS[deg] was changed with Cell lengths [ang] (https://github.com/aiidateam/aiida-cp2k/blob/ffe2943f287e3c22488f04847efb7aa016ce9ba7/aiida_cp2k/utils/parser.py#L256-L260).

One should check one by one all the string matches and see if other changes were made.

mpougin commented 3 years ago

Thank you for your help @ltalirz and @danieleongari. For a quick solution, I will use cp2k 5.1 for now. @danieleongari do you want me to check if there are further incompabilities or will you take care of that?

ltalirz commented 3 years ago

@mpougin if you don't mind it would be great if you could check whether that fixes things. I described above how you can go about this (without rerunning the calculation) - you can make changes in the parser one by one until the result of the parsing is as you expect

P.S. Rather than using the verdi shell, perhaps it's better to write things in a script that you run since you would need to restart the shell whenever you make changes to the parser in order to pick up the changes in the module.

P.P.S. When it gets to the point of making the PR to aiida-cp2k, please have a copy of the cp2k output with v8.1 and v5.1 ready - we will then add a test that makes sure parsing works in both cases.

danieleongari commented 3 years ago

@ltalirz @mpougin I can take care of it: the way it helped me to create the advanced parser was to create ENERGY, GEO_OPT, CELL_OPT, MD-NVT, and MD-NPT_F outputs and be sure that the parser was working as expected for all of them. That is why the logic of the parser is not so intuitive: there are quite some caveats in dealing for all of these 5 cases. Now, we would need to make the same for both versions of CP2K (or the more possible) and I agree it is a good idea to include them in the plugin as tests. I hope they did not introduce some other major differences that would make the logic for an universal parser too complicate. It will take some time but I can make it by the end of the week.

mpougin commented 3 years ago

Thank you @danieleongari ! I will work with cp2k 5.1 for now then. Or is there any major improvement in the 6.1version? I didn't have it installed in AiiDA yet, but I would use that then temporary

danieleongari commented 3 years ago

I think you are also using 6.1: unfortunately stuff are a bit confusing, since SCITAS updated CP2K to 6.1 keeping the name of the module 5.1. There should be no big differences and I don't know when they made the update exactly, but if you inspect the output you should read that the version is 6.1

mpougin commented 3 years ago

thanks for you explanations @danieleongari. I checked my output and it's still 5.1. But if you say that there no big differences, I will stay with that for now as it's running fine and would (hopefully) only be temporary anyways

danieleongari commented 3 years ago

You are right it is 5.1 on both fidis and helvetios, sorry for the confusion. I remember this 5.1/6.1 ambiguity but maybe I dreamed it: so far everything was done and tested on 5.1

danieleongari commented 3 years ago

Fixed in https://github.com/aiidateam/aiida-cp2k/pull/142

mpougin commented 3 years ago

@ltalirz @danieleongari , is this solved now? I am confused as the PR is still open

danieleongari commented 3 years ago

Solved in 0999ccec3e445cfd0dfd37a65ab013299a5f7d51

mpougin commented 3 years ago

thank you for clarification