Fix/matdyn phlabels

[mkotiuga]

supersedes #686

[mbercx]

This PR superseded #686

@sphuber I'm first going to clean this up and make sure it's properly rebased on develop. :)

[mbercx]

Changes to regression test files look good in my opinion, i.e. the expected parsed symmetry labels are added. We should still add a test to see if multiple separate PhCalculations in one PhBaseWorkChain are added properly.

[mbercx]

The code is already in good shape I think (tests still need to be fixed though), but it seems I can't just override the output_parameters output with the self.out() method. @sphuber is there a clean way of doing this?

[sphuber]

but it seems I can't just override the output_parameters output with the self.out() method. @sphuber is there a clean way of doing this?

~~Don't understand what you mean here. What line are you referring to in the changes and what is the problem?~~

Never mind, I see the problem now. You want to add the result of the merge_ph_output and add that as the exposed output_parameters output, replacing the one returned by the last completed PhCalculation.

Indeed, the way you are doing it now won't work, since when you run self.results it will already attach the output_parameters node and you cannot replace it then in the step after it. The only solution I currently see is to override the results step of the base work chain, which is not ideal because we make the code fragile and dependent on changes in aiida-core.

Ideally, the BaseRestartWorkChain provides a hook, like get_outputs, that is called in results and returns the final output mapping. You could then just override this and the rest of results would remain the same. As for now, I think we do it here by overriding results entirely and add a feature request in aiida-core to add this and then in the next release we can improve the implementation here. Thoughts?

[mbercx]

Ideally, the BaseRestartWorkChain provides a hook, like get_outputs, that is called in results and returns the final output mapping. You could then just override this and the rest of results would remain the same. As for now, I think we do it here by overriding results entirely and add a feature request in aiida-core to add this and then in the next release we can improve the implementation here. Thoughts?

Sure, the hook approach would work. 👍 For this PR, maybe we can just call the create_merged_output step first, so the output_parameters are already set before the results?

[sphuber]

Sure, the hook approach would work. 👍 For this PR, maybe we can just call the create_merged_output step first, so the output_parameters are already set before the results?

Does that work? As in the second time it is being added it simply is ignored without error (would surprise me tbh)? If so, that works as well and is less messy, but it would require a big fat comment that it is relying on this.

[mbercx]

As in the second time it is being added it simply is ignored without error (would surprise me tbh)?

I think I tried overriding it in the merge step, after the results one, and I don't think it raised an error. But I'll give it a go. ^^

[mbercx]

Seems first setting the output_parameters in the create_merged_output step and then adding the results step to the outline actually works. The following work chain:

$ verdi process status 9696
PhBaseWorkChain<9696> Finished [0] [4:results]
    ├── PhCalculation<9699> Finished [400]
    ├── PhCalculation<9705> Finished [0]
    └── merge_ph_outputs<9709> Finished [0]

Will nicely have the merged output parameters:

In [1]: ph_base = load_node(9696)

In [2]: ph_base.outputs.output_parameters.get_dict()
Out[2]: 
{'wall_time': '     55.47s ',
 'num_q_found': 2,
 'number_of_atoms': 1,
 'number_of_qpoints': 4,
 'wall_time_seconds': 113.61,
 'dynamical_matrix_1': {'q_point': [0.0, 0.0, 0.0],
  'frequencies': [-3.205876, -3.194079, -3.194079],
  'point_group': 'D_3d (-3m)',
  'mode_symmetry': ["L_2'", "L_3'", "L_3'"],
  'q_point_units': '2pi/lattice_parameter',
  'frequencies_units': 'cm-1'},
 'dynamical_matrix_2': {'q_point': [0.0, 0.0, -0.612372436],
  'frequencies': [65.862765, 65.862765, 112.146012],
  'point_group': 'D_3d (-3m)',
  'mode_symmetry': ["L_3'", "L_3'", "L_2'"],
  'q_point_units': '2pi/lattice_parameter',
  'frequencies_units': 'cm-1'},
 'dynamical_matrix_3': {'q_point': [0.0, -0.577350269, 0.204124145],
  'frequencies': [65.862769, 65.863417, 112.148301],
  'point_group': 'C_2h (2/m)',
  'mode_symmetry': ['A_u', 'B_u', 'B_u'],
  'q_point_units': '2pi/lattice_parameter',
  'frequencies_units': 'cm-1'},
 'dynamical_matrix_4': {'q_point': [0.0, -0.577350269, -0.40824829],
  'frequencies': [83.202774, 83.20789, 105.820001],
  'point_group': 'C_2h (2/m)',
  'mode_symmetry': ['A_u', 'B_u', 'B_u'],
  'q_point_units': '2pi/lattice_parameter',
  'frequencies_units': 'cm-1'},
 'number_of_irr_representations_for_each_q': [2, 2, 3, 3]}

@sphuber so I can use this workaround for now, and open a new issue/PR that would rely on https://github.com/aiidateam/aiida-core/pull/5618 once this feature has been released.

@mkotiuga I've given the code a try, and it seems to work fine. Could you also give it a go, and closely check how the outputs are formatted to see if you still have any comments? E.g. should mode_symmetry perhaps be called mode_symmetries? ^^

[mbercx]

PR is updated to use the feature implemented in https://github.com/aiidateam/aiida-core/pull/5618, but unfortunately this hasn't been released yet. @sphuber any chance we can push for a v2.1 soonish?

Also added a test, which fails here because of said feature not being released in aiida-core. We should really write a tutorial on writing tests in AiiDA, it's typically more difficult than implementing the actual feature. 😝

@mkotiuga in the meanwhile you'll have to work on the cutting edge of aiida-core development.

[mbercx]

@sphuber quick question: what's the use of the generate_parser fixture?

@pytest.fixture(scope='session')
def generate_parser():
    """Fixture to load a parser class for testing parsers."""

    def _generate_parser(entry_point_name):
        """Fixture to load a parser class for testing parsers.

        :param entry_point_name: entry point name of the parser class
        :return: the `Parser` sub class
        """
        from aiida.plugins import ParserFactory
        return ParserFactory(entry_point_name)

    return _generate_parser

This seems to just import the parser class, no? Can't we just remove this fixture and directly load the parser class?

[sphuber]

This seems to just import the parser class, no? Can't we just remove this fixture and directly load the parser class?

One would think yeah :sweat_smile: Saves an explicit import in most test files, but that is not really good enough, fine to remove it.

[sphuber]

@sphuber any chance we can push for a v2.1 soonish?

I am trying my hardest. Almost everything is there. Just waiting for a final release of plumpy and kiwipy, but for the latter we need an important fix, which I am not sure how fast we can deliver.

[sphuber]

@mbercx aiida-core==2.1 is out

[mbercx]

@mkotiuga could you double check that the point group and symmetry labels for the non-symmetry case are to your liking, and maybe how we could expand on the comment here a bit?

https://github.com/mkotiuga/aiida-quantumespresso/blob/94d6839756ae1810fd581672cbed38cf44093977/src/aiida_quantumespresso/parsers/parse_raw/ph.py#L209-L212

@sphuber still had to make some fixes to cover all the different variants of stdout for ph.x. With aiida-core v2.1 released, I think this one is almost good to go.

[mbercx]

@sphuber any idea why the integration CI test is now failing to install the SSSP? 🤔

[sphuber]

@sphuber any idea why the integration CI test is now failing to install the SSSP? thinking

It's because the CLI is broken with aiida-core==2.1, see this issue: https://github.com/aiidateam/aiida-pseudo/issues/130 I will fix it now and make a release that is compatible with v2.1. The same problem applies to aiida-quantumespresso. I already made a branch with the fix long ago (https://github.com/aiidateam/aiida-quantumespresso/tree/fix/834/cli-verdi-context) was just waiting for the release to happen.

[sphuber]

Ok, aiida-pseudo==0.8.0 is released and I updated main branch to add the dependency requirement. You can rebase now and tests should run without problems.

[mkotiuga]

When a PhBaseCalculation for 1 q-point has a PhCalculation that hits the wall time and then properly restarts, the final mode symmetries are not parsed. This is probably due to some logic in parses/parse_raw/ph.py - will try look into it shortly

[mkotiuga]

When a PhBaseCalculation has multiple PhCalcuations and the wall-time is hit mid q-point calculation there is an extra entry in number_of_irr_representations_for_each_q as there are two 'headers' for the same q-point in two different PhCalculations - This is probably also due to an error in the parser

[mbercx]

It took a lot of tweaking, but I think the PR is now very close to being finalised. I think the most work now is to write a proper commit message. ^^

The parsing can now deal with:

• Calculations that finish in one go for multiple q-points or a single one.
• Calculations that are restarted between q-points.
• Calculations that are restarted while running a single q-point.

In all cases, the outputs are identical, which should be the goal.

[mbercx]

@mkotiuga I've updated the title and OP to what I think the commit header and message should be. Have a look and let me know if you have any comments!

[mbercx]

@sphuber let me know if you still want to have a look at this PR, but hopefully my beautiful and extensive testing convinces you that this is quality code. 😁

[sphuber]

Thanks @mbercx . I don't have the time to go through all of it, but indeed from first glance, this looks like quality code. Happy to approve it if you can update the branch and all tests pass

[mkotiuga]

@mbercx looks good