aiidateam / aiida-quantumespresso

The official AiiDA plugin for Quantum ESPRESSO
https://aiida-quantumespresso.readthedocs.io
Other
52 stars 77 forks source link

BUG in the ions sections in case tehre are no settings: I set fixed coords to None #957

Closed cpignedoli closed 11 months ago

cpignedoli commented 1 year ago

at line 278 fixed_coords = settings.get('FIXED_COORDS', None) may be accessed before settings is defined.

Example of crash:

1 LOG MESSAGES: +-> REPORT at 2023-06-17 04:00:21.513690+00:00 | [16139|PwCalculation|on_except]: Traceback (most recent call last): | File "/home/jovyan/.local/lib/python3.9/site-packages/plumpy/process_states.py", line 228, in execute | result = self.run_fn(self.args, self.kwargs) | File "/home/jovyan/.local/lib/python3.9/site-packages/aiida/engine/processes/calcjobs/calcjob.py", line 675, in parse | exit_code_retrieved = self.parse_retrieved_output(retrieved_temporary_folder) | File "/home/jovyan/.local/lib/python3.9/site-packages/aiida/engine/processes/calcjobs/calcjob.py", line 786, in parse_retrieved_output | exit_code = parser.parse(**parse_kwargs) | File "/home/jovyan/.local/lib/python3.9/site-packages/aiida_quantumespresso/parsers/pw.py", line 151, in parse | exit_code = validator(trajectory, parsed_parameters, logs_stdout) | File "/home/jovyan/.local/lib/python3.9/site-packages/aiida_quantumespresso/parsers/pw.py", line 244, in validate_ionic | if not self.is_ionically_converged(trajectory): | File "/home/jovyan/.local/lib/python3.9/site-packages/aiida_quantumespresso/parsers/pw.py", line 278, in is_ionically_converged | fixed_coords = settings.get('FIXED_COORDS', None) | UnboundLocalError: local variable 'settings' referenced before assignment

sphuber commented 1 year ago

Thanks @cpignedoli . Think you may have accidentally committed with black in the pre-commit. Resulting in a lot of unnecessary changes that will fail our pre-commit checks.

sphuber commented 1 year ago

I have undone the linting changes and pushed the fix. Since there were so many changes, it was difficult to see which ones were actually intended. Could you please have a look that I got the changes right and maybe try the branch to make sure? Thanks!

cpignedoli commented 1 year ago

Hi @sphuber I apologize yes I did not pay attention to the linting. What you did is perfect, tahnks a lot, line 278 is replaced now by

    if 'settings' in self.node.inputs:
        settings = _uppercase_dict(self.node.inputs.settings.get_dict(), dict_name='settings')
        fixed_coords = settings.get('FIXED_COORDS', None)
    else:
        fixed_coords = None
yakutovicha commented 1 year ago

Hi everyone 👋

I think @cpignedoli assumed that @mbercx is going to implement the proposed change along with the test.

Not sure what was the final agreement, but I took the liberty to make the change. @cpignedoli is on holiday, so if anything else needs to be done - I can give a hand.

@mbercx, how shall we continue here?

yakutovicha commented 1 year ago

A side remark: I tested the current version of the PR, and for our tests, it seems to work fine 👍