Skip to content

Sort atom according to the type_map in Multisystem#109

Merged
amcadmus merged 4 commits into
deepmodeling:develfrom
zezhong-zhang:devel
Aug 11, 2020
Merged

Sort atom according to the type_map in Multisystem#109
amcadmus merged 4 commits into
deepmodeling:develfrom
zezhong-zhang:devel

Conversation

@zezhong-zhang

@zezhong-zhang zezhong-zhang commented Aug 10, 2020

Copy link
Copy Markdown
Contributor
  1. Follow the input type_map for Multisystem instead of alphabetic order.
  2. Make the formula in the order of the type_map.

At present, the order will change when calling the check_atom_names.

@amcadmus amcadmus requested a review from njzjz August 10, 2020 11:53
@amcadmus

Copy link
Copy Markdown
Member

It is strange that python 3.6-dev passed the unit tests, but 3.5, 3.5-dev and 3.6 failed...

@njzjz

njzjz commented Aug 10, 2020

Copy link
Copy Markdown
Member

It’s the unit test that @felix5572 added.

Comment thread dpdata/system.py
if len(new_in_system):
# A new atom_name appear, add to self.atom_names
self.atom_names.extend(new_in_system)
self.atom_names.sort()

@njzjz njzjz Aug 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am wondering whether we need to sort atom names when self.atom_names does not have the enough elements?

@felix5572

Copy link
Copy Markdown
Contributor

I have make a pr in your repo, this bug would be fixed. Please take a look. @zezhong-zhang

bug fix to help your pull request && modify  set subtraction
@zezhong-zhang

Copy link
Copy Markdown
Contributor Author

I have make a pr in your repo, this bug would be fixed. Please take a look. @zezhong-zhang

Many thanks for your help, pr now merged in my repo. @felix5572

@amcadmus amcadmus merged commit 73e9cf9 into deepmodeling:devel Aug 11, 2020
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