Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix post_fp_vasp and make_fp_configs for simplify #803

Merged
merged 4 commits into from
Jul 19, 2022

Conversation

HuangJiameng
Copy link
Collaborator

@HuangJiameng HuangJiameng commented Jul 15, 2022

When job.json is empty, it won't be generated. But post_fp_vasp will call it without checking if it exists. see [https://github.com//issues/794](issue #794)
Besides, there's something wrong with naming task directories. dpdata has to append all different systems into one set, which causes RuntimeError('systems with inconsistent formula could not be append: %s v.s. %s' % (self.uniq_formula, system.uniq_formula)). Now those problems are fixed.

@njzjz
Copy link
Member

njzjz commented Jul 15, 2022

instead, we should change LabeledSystem to MultiSystems.

@wanghan-iapcm wanghan-iapcm requested a review from njzjz July 16, 2022 03:22
jj = 0
for system in systems:
for subsys in system:
task_name = "task." + fp_task_fmt % (0, jj)
task_name = "task." + fp_task_fmt % (ii, jj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, I suggest using MultiSystems to append systems, like what dpgen2 does, so there will be no errors to add different systems.
https://github.com/deepmodeling/dpgen2/blob/008d1dfe91f3628a7039dca5d3af10c1b4bca506/dpgen2/op/collect_data.py#L67-L73

@HuangJiameng
Copy link
Collaborator Author

But here has already used MultiSystems.

systems = dpdata.MultiSystems(
*[system(s, fmt='deepmd/npy') for s in system_paths])

@njzjz
Copy link
Member

njzjz commented Jul 18, 2022

here

dpgen/dpgen/generator/run.py

Lines 3135 to 3138 in 3c6803b

if all_sys is None:
all_sys = _sys
else:
all_sys.append(_sys)

@HuangJiameng
Copy link
Collaborator Author

Many operations are dependent on LabeledSystem. Maybe not to change it?

@njzjz
Copy link
Member

njzjz commented Jul 18, 2022

ok, I agree, although it's not a good behavior...

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2022

Codecov Report

Merging #803 (121fba6) into devel (78c8ec5) will decrease coverage by 0.00%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##            devel     #803      +/-   ##
==========================================
- Coverage   35.13%   35.13%   -0.01%     
==========================================
  Files          96       96              
  Lines       16804    16807       +3     
==========================================
+ Hits         5904     5905       +1     
- Misses      10900    10902       +2     
Impacted Files Coverage Δ
dpgen/simplify/simplify.py 0.00% <0.00%> (ø)
dpgen/generator/run.py 63.69% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78c8ec5...121fba6. Read the comment docs.

@AnguseZhang AnguseZhang merged commit e4e9053 into deepmodeling:devel Jul 19, 2022
@HuangJiameng HuangJiameng deleted the fix_post_for_simplify branch September 26, 2022 08:24
njzjz added a commit to njzjz/dpgen that referenced this pull request Apr 16, 2023
deepmodeling#803 changed the behavior of sys_idx in the fp step and caused there to be lots of systems. However, it failed to try to get the batch size of these systems.
wanghan-iapcm pushed a commit that referenced this pull request Apr 21, 2023
#803 changed the behavior of sys_idx in the fp step and caused there to
be lots of systems. However, it failed to try to get the batch size of
these systems.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

4 participants