Feature/mobility #23
Feature/mobility #23
Conversation
LIU-Binbin
force-pushed
the
feature/mobility
branch
2 times, most recently
from
33df0de to
0a135a0
Compare
Note: this commit introduces mobility workflows. Please note that the associated parser (parsers_mobility.py) has been excluded pending testing, but prep.py may already contain references to the logic intended for it.
LIU-Binbin
force-pushed
the
feature/mobility
branch
from
0a135a0 to
b9d6c48
Compare
| # ) | ||
| # inputs.ph.parent_folder = scf_base_wc.outputs.remote_folder | ||
|
|
||
| if 'parent_folder_ph' in self.inputs and self.inputs.parent_folder_ph.creator.process_label == "PhCalculation": |
There was a problem hiding this comment.
I didn't see a parent_folder_ph in the input port of EpwPrepWorkChain. Did you add it? Besides since the history is a bit messy I'm not sure whether this part is added in this PR or from the previous PR. If it is a new feature that you want to introduce in this PR, since the restart of PhCalculation is not very relevant to the mobility function. Can you wait for the next PR to do it? If it's my previous PR, then just leave it here.
| "always_run_final", valid_type=orm.Bool, default=lambda: orm.Bool(False) | ||
| ) | ||
| spec.input( | ||
| "quadruple_dir", valid_type=orm.Str, # orm.RemoteData, orm.RemoteStashFolderData) |
There was a problem hiding this comment.
Here the "quadruple_dir" is an AiiDA string. But in the EpwCalculation it is defined as optional to an orm.Str OR orm.RemoteData. I didn't find anywhere that it is used as a orm.RemoteData. If it's always a str, can you modify the definition in EpwCalculation? Actually I think orm.RemoteData is better than a plain orm.Str but of course it's up to you.
| ) | ||
| spec.input( | ||
| "quadruple_dir", valid_type=orm.Str, # orm.RemoteData, orm.RemoteStashFolderData) | ||
| required=False, |
There was a problem hiding this comment.
Can you remove the commented codes?
|
|
||
| def should_run_conv_test(self): | ||
| """Return True if there are still distances left in the list to test.""" | ||
| # 只要列表不为空，就返回 True，继续进入 run_conv |
There was a problem hiding this comment.
We'd better use English everywhere so that everyone can understand
| running = self.submit(EpwBaseWorkChain, **inputs) | ||
| self.report(f"Launched EpwBaseWorkChain<{running.pk}> for convergence run #{self.ctx.iteration}") | ||
|
|
||
| return ToContext(epw_interp=append_(running)) |
There was a problem hiding this comment.
Could you please refer to this PR #18 and substitute all ToContext function with a plain python dictionary? I tested it and they should be identical
|
|
||
| # === 处理 Quadrupole 文件的软链接 (ln -s) === | ||
| # 假设你的 WorkChain 输入里有名为 'quadruple_dir' 的 Str 类型的绝对路径 | ||
| if 'quadruple_dir' in self.inputs: |
There was a problem hiding this comment.
If I'm not mistaken this is already implemented in EpwCalculation? If so please remove the duplicated logic here.
| return builder | ||
|
|
||
|
|
||
| def generate_reciprocal_points(self): |
There was a problem hiding this comment.
We can retrieve the qpoints and kpoints_nscf from parent_folder_epw. So I think it's better to avoid regeneration of qpoints and kpoints_nscf. For kpoints_scf it's a bit hard to track. But I didn't see a reason that kpoints_scf needs to be re-generated? Do you use it in the following part of this workflow?
| # Check for convergence, assuming mobility is a dictionary with values to compare. | ||
| # This part might need adjustment depending on the exact structure of `mobility_iBTE`. | ||
| # For simplicity, let's assume it's a single value for now. | ||
| is_converged = abs(prev_mobility - new_mobility) < self.inputs.convergence_threshold.value |
There was a problem hiding this comment.
In SuperConWorkChain I forgot to use relative change of Tc. I'm not sure what's the best way to define the convergence of mobility. Maybe you can consider to use relative change of mobility if it's better
| inputs.epw.metadata.options['remote_symlink_list'] = [symlink_item] | ||
| # =============================================== | ||
|
|
||
| if 'kfpoints_factor' in self.inputs: |
There was a problem hiding this comment.
There seems no definition of kfpoints_factor in the input namespace of this workchain. And I would suggest to define the default value in the definition of input port instead of putting it here.
|
|
||
| inputs = AttributeDict(self.exposed_inputs(EpwBaseWorkChain, namespace="epw_mobility")) | ||
| inputs.parent_folder_epw = self.inputs.parent_folder_epw | ||
| inputs.qfpoints_distance = self.ctx.interpolation_list.pop(0) # Take the smallest distance first |
There was a problem hiding this comment.
Usually we gradually increase the qpoints/kpoints mesh to converge a quantity. But here the order is reversed?
|
Hi Binbin, Thanks for the work! It's great to see the mobility functionality included in our workflow. I noticed that because I recently squashed and merged several PRs, some 'ghost commits' have appeared in this PR. For example, it shows changes from my previous PR in src/aiida_epw/tools/parsers.py that are already in main. Could you please rebase your branch against the latest main to clean up the history? Sorry for the inconvenience. I've also added my review comments below. Hope they help! |
| - 0.03 | ||
| convergence_threshold: 0.5 | ||
| epw_mobility: | ||
| metadata: |
There was a problem hiding this comment.
There is name conflict between metadata of EpwBaseWorkChain and EpwCalculation so we should move options to the top level. See this PR #21
There was a problem hiding this comment.
Where is this file used?
2 participants
Description
This PR introduces the Mobility workchain and associated protocols
mobility.yamlandbase_mob.yaml.Important Note:
The prep.py workflow has been updated to include logic for mobility calculations. However, the associated parser file (
src/aiida_epw/tools/parsers_mobility.py) is excluded from this PR as it is currently undergoing testing.Please be aware that while prep.py references mobility parser logic, the actual parser implementation is not included in this commit.