aiidateam / aiida-quantumespresso

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

Adding logic to PhBaseWorkChain to generate mesh from structure and qpoints_distance #956

Closed AndresOrtegaGuerrero closed 11 months ago

AndresOrtegaGuerrero commented 1 year ago

This PR addresses #954

AndresOrtegaGuerrero commented 1 year ago

@sphuber Thank you for your suggestions, I will organize this PR, to include all of them. Let me know your opinion of the creation of a function in the workchain validate_qpoints in the workchain. I am a lil skeptical to include qpoints as input of the workchain (Like in PwBaseWorkchain). I guess if these new inputs we discussed are added, I should also consider a test for it as well.

AndresOrtegaGuerrero commented 1 year ago

For a new protocol using distance I have done some test with the functioncreate_kpoints_from_distance , we could use qpoints_distance = 0.6 for fast, qpoints_distance = 0.3 for moderate (Default), and qpoints_distance = 0.2 precise.

mbercx commented 1 year ago

I was about to implement this, but you already did it. Is there something important on why this is not yet merged? @sphuber @mbercx ?

I must admit when I see @sphuber started reviewing a PR I stop paying attention myself. ^^

Would be good to have this feature indeed. @sphuber @AndresOrtegaGuerrero happy to help if needed.

sphuber commented 1 year ago

I was simply waiting for an update after the review. @AndresOrtegaGuerrero please let me know if you are done with the changes and I will review it again.

AndresOrtegaGuerrero commented 1 year ago

@sphuber @mbercx @bastonero . Hello , I actually stoped working on it since other things came first. I included the validator. I saw a similar one for the PwBandsWorkChain, yet i didnt see the test? any suggestions ?. On the other hand, @bastonero could you test if the branch is behaving as expected ?

bastonero commented 1 year ago

Hello @AndresOrtegaGuerrero, thanks a lot for prioritizing this!

On the other hand, @bastonero could you test if the branch is behaving as expected ?

I don't know if I can push commits to your empa branch. Nevertheless, you can proceed as follows: pip install -e .[tests] from the aiida-quantumespresso folder Then add something like this into tests/workflows/ph/test_base.py

@pytest.mark.usefixtures('aiida_profile')
def test_invalid_inputs(generate_workchain_ph, generate_inputs_ph):
    """Test `PhBaseWorkChain` validation methods."""
    inputs ={'ph': generate_inputs_ph()}
    # modify inputs to make them invalid - e.g. inputs['qpoints_distance'] = orm.Float(0.2) ?
    message = r'WRITE HERE THE ERROR MESSAGE YOU EXPECT'
    with pytest.raises(ValueError, match=message):
        generate_workchain_ph(inputs=inputs)

Then do

cd tests/workflows/ph/
pytest -sv test_base.py

And check everything is working. If not, you can change the code where it complains and run the tests again, till they succeed. Then you should probably pull from the main branch, as I added new tests for the ph protocols. Since now you changed it to have the qpoints_distance, you will have to just change the files for the regression tests (i.e. tests/workflows/protocols/ph/test_base/test_default.yaml). See also how I have done it (https://github.com/aiidateam/aiida-quantumespresso/commit/39287e03cb6bbf1915662685a5c441e9c7c36030). I think it's useful for you to learn how to do tests, as it is the best and fastest way to develop in aiida, and avoids to run actual jobs. So check also the tests for the protocol, i.e. test everything in tests/workflows/protocols/ph.

Let me know if you have any trouble, and I'll try to help out!

AndresOrtegaGuerrero commented 1 year ago

I am still addressing the tests, i will request review once is ready. I did push but is not ready sorry about that

AndresOrtegaGuerrero commented 1 year ago

@bastonero thank you i will address your comments!

AndresOrtegaGuerrero commented 12 months ago

@mbercx let me know if there is another change i should do ?