Skip to content

Add post-processing base workchain#1174

Closed
edan-bainglass wants to merge 1 commit into
aiidateam:support-release/4.15.0from
edan-bainglass:pp-workchain
Closed

Add post-processing base workchain#1174
edan-bainglass wants to merge 1 commit into
aiidateam:support-release/4.15.0from
edan-bainglass:pp-workchain

Conversation

@edan-bainglass
Copy link
Copy Markdown
Member

This PR registers a simple base restart wrapper on the PpCalculation (simplified version of the one implemented by @AndresOrtegaGuerrero here. We can always expand on it down the road. Mostly opening this PR to tie together several components required for a PREMISE deliverable.

Supersedes #1173

@mbercx
Copy link
Copy Markdown
Member

mbercx commented Sep 30, 2025

Thanks @edan-bainglass! Will do a full review later, but first some quick questions (while you are potentially still awake 😉):

  1. I'm not very familiar with pp.x, can it benefit from error recovery? I've already wanted to push down the get_builder_from_XXX concept down to the CalcJob classes (see Protocols: Extend protocol concept to CalcJob classes #947) and I'm wondering if pp.x isn't a good case for this.
  2. Can I ask why you didn't use the get_builder_from_protocol() approach? Now users can't e.g. specify overrides. Perhaps pp.x is so simple that this isn't necessary?

@edan-bainglass
Copy link
Copy Markdown
Member Author

pp.x does not use protocols. And yes, it is relatively simpler. I used the get from quantity method to provide a simple interface for constructing the inputs by specifying what you want to compute. However, pp.x salad takes settings, which can be used to modify the inputs. This would be there next thing to implement, in a later PR. I'd also like @AndresOrtegaGuerrero to have a look, to get a sense of the direction this would go, and indeed, of a restart can ever be useful. But since this is entirely new, starting small should be fine. Let's see.

@AndresOrtegaGuerrero
Copy link
Copy Markdown
Collaborator

As @mbercx pointed out, the post-processing (pp) calculation does not benefit from a restart. However, what I do find useful is having a workflow that can generate as many plots or properties as requested, while also collecting all the relevant outputs in one place. Since we already have access to the wavefunction, the most efficient approach is to extract everything in a single run.

I could imagine using the overrides mechanism to specify additional properties. Given that there are many possible properties—each depending on knowledge of both the wavefunction and the system—it’s difficult to define fixed protocols in advance.

What might be valuable, though, is to include error checks. For example, based on the properties requested and the parameters of the PwCalculation that was passed, the workflow could verify whether the settings for pp.x are compatible with those of the preceding PwCalculation.

@mbercx
Copy link
Copy Markdown
Member

mbercx commented Oct 1, 2025

Thanks for the comments @AndresOrtegaGuerrero! Valuable info.

However, what I do find useful is having a workflow that can generate as many plots or properties as requested, while also collecting all the relevant outputs in one place.

I like that idea. 👍 Maybe the workflow has a parent_folder for the preceding pw.x, and a properties input that is a list of strings, corresponding to the PostProcessQuantity @edan-bainglass introduced? Is that similar to the workflow you developed?

I could imagine using the overrides mechanism to specify additional properties. Given that there are many possible properties—each depending on knowledge of both the wavefunction and the system—it’s difficult to define fixed protocols in advance.

I can see how the concept of "protocols" as we defined them for the pw.x calculations doesn't really make sense here. Probably an approach like @edan-bainglass introduced is more sensible. Initially I was keen on keeping the usage consistent between classes, but since it's a different concept we are creating a builder from I think having a different method name makes sense to me.

What might be valuable, though, is to include error checks.

I suppose what you mean here is validation? That could certainly be possible, both for the PpCalculation and PpPropertiesWorkChain (working title 😉 ). At least if the creator of the parent_folder is in the database as well.


What I would propose is to push the get_builder_from_quantity method down to the PpCalculation, and then develop such a PpPropertiesWorkChain (not inheriting from BaseRestartWorkChain) that does what you describe. Probably we'd have a PpPropertiesWorkChain.get_builder_from_properties method or something along those lines. Does that make sense?

@edan-bainglass you mentioned this was important for a PREMISE deliverable. What exactly do you need for this? Would the above suggestion work for your use case as well? E.g. having the get_builder_from_property method on the PpCalculation?

@edan-bainglass
Copy link
Copy Markdown
Member Author

edan-bainglass commented Oct 1, 2025

Fantastic input from both. Thank you 🙏🙏

@mbercx for PREMISE, I developed a CommonPostProcessWorkChain in AiiDA Common Workflows (ACWF) (aiidateam/aiida-common-workflows#342) to support a new composite common workflow for AFM simulations, which requires SCF + post-processing (aiidateam/aiida-common-workflows#343). The only requirement from aiida-quantumespresso is that PP is available via a registered WorkChain. So the above suggestions will work, and I'm in favor of all suggestions. Happy to allocate some time to updating this. Once the WorkChain is set, it's just a matter of updating the ACWF workflow, if even needed.

As for the name, maybe PpWorkChain would be enough. There is no other. It would accept one or more properties by default.

@mbercx
Copy link
Copy Markdown
Member

mbercx commented Oct 1, 2025

Great, thanks @edan-bainglass! Also glad to see some activity on the common workflows. Happy to prioritize review once it's ready.

@mbercx mbercx deleted the branch aiidateam:support-release/4.15.0 October 2, 2025 08:52
@mbercx mbercx closed this Oct 2, 2025
@mbercx
Copy link
Copy Markdown
Member

mbercx commented Oct 2, 2025

@edan-bainglass apologies, I only now noticed this PR was into the support-release/v4.x branch, not the support/v4.x. I'm going to have to rewrite a bit of history there, so wait a moment, but I'll ping you and then you can rebase off that one.

@mbercx
Copy link
Copy Markdown
Member

mbercx commented Oct 2, 2025

Alright, support/v4.x is released, so will be stable. Feel free to rebase on that, and then adapt the PR to target that one, or maybe open a new one if that is not possible. 🙏

@edan-bainglass
Copy link
Copy Markdown
Member Author

Thanks @mbercx. I'll do this tomorrow morning.

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.

3 participants