search

logsdail / carmm

Scripts for creation, manipulation and analysis of geometric and electronic structure of molecular models
GNU General Public License v3.0
5 stars 18 forks source link

Assertion for surface-adsorbate distance in bond_length_scan #122

Closed ikowalec closed 1 year ago

codecov[bot] commented 1 year ago

Codecov Report

Merging #122 (4b69c49) into master (27d508f) will increase coverage by 0.23%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #122      +/-   ##
==========================================
+ Coverage   83.84%   84.08%   +0.23%     
==========================================
  Files          67       67              
  Lines        2786     2790       +4     
==========================================
+ Hits         2336     2346      +10     
+ Misses        450      444       -6
Flag Coverage Δ
unittests 84.08% <100.00%> (+0.23%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
carmm/build/neb/bond_length_scan.py 94.11% <100.00%> (+11.09%) :arrow_up:
examples/build_dissociation.py 100.00% <100.00%> (ø)
logsdail commented 1 year ago

OK - does that introduce a minimum dependence from us on the ASE version number? This is something we need to consider anyway at some point for the calculator, I guess...

From: Igor Kowalec @.> Sent: 09 August 2023 13:52 To: logsdail/carmm @.> Cc: Andrew Logsdail @.>; Review requested @.> Subject: Re: [logsdail/carmm] Assertion for surface-adsorbate distance in bond_length_scan (PR #122)

External email to Cardiff University - Take care when replying/opening attachments or links. Nid ebost mewnol o Brifysgol Caerdydd yw hwn - Cymerwch ofal wrth ateb/agor atodiadau neu ddolenni.

@ikowalec commented on this pull request.

In carmm/build/neb/bond_length_scan.pyhttps://github.com/logsdail/carmm/pull/122#discussion_r1288435817:

@@ -81,12 +86,12 @@ def take_second(n):

 for i in range(1, n_steps+1):
     # operate on a deepcopy for intended functionality

Yes, in previous versions atoms.copy() was providing a shallow copy and now it is a completely new instance, so using copy library can be avoided

— Reply to this email directly, view it on GitHubhttps://github.com/logsdail/carmm/pull/122#discussion_r1288435817, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFMK4MYKIXEENPWLMXRFRPLXUOB2BANCNFSM6AAAAAA3JYVWPQ. You are receiving this because your review was requested.Message ID: @.***>

ikowalec commented 1 year ago

OK - does that introduce a minimum dependence from us on the ASE version number? This is something we need to consider anyway at some point for the calculator, I guess... ____ From: Igor Kowalec @.> Sent: 09 August 2023 13:52 To: logsdail/carmm @.> Cc: Andrew Logsdail @.>; Review requested @.> Subject: Re: [logsdail/carmm] Assertion for surface-adsorbate distance in bond_length_scan (PR #122) External email to Cardiff University - Take care when replying/opening attachments or links. Nid ebost mewnol o Brifysgol Caerdydd yw hwn - Cymerwch ofal wrth ateb/agor atodiadau neu ddolenni. @ikowalec commented on this pull request. ____ In carmm/build/neb/bond_length_scan.py<#122 (comment)>: @@ -81,12 +86,12 @@ def take_second(n): for i in range(1, n_steps+1): # operate on a deepcopy for intended functionality - atoms = copy.deepcopy(atoms) + atoms = atoms.copy() Yes, in previous versions atoms.copy() was providing a shallow copy and now it is a completely new instance, so using copy library can be avoided — Reply to this email directly, view it on GitHub<#122 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFMK4MYKIXEENPWLMXRFRPLXUOB2BANCNFSM6AAAAAA3JYVWPQ. You are receiving this because your review was requested.Message ID: @.***>

Re: codebase, the deepcopy can copy the calculator, but it is generally frowned upon as per: https://gitlab.com/gpatom/ase-gpatom/-/issues/8 For copying Atoms objects we should be using the dedicated Atoms.copy() function. It drops the calculator altogether, so a new one might have to be generated if required.

logsdail commented 1 year ago

OK, thanks for clarifying. I think historically .copy() returned a pointer not a new object, hence this legacy exists in our code!

From: Igor Kowalec @.> Sent: 09 August 2023 14:34 To: logsdail/carmm @.> Cc: Andrew Logsdail @.>; Review requested @.> Subject: Re: [logsdail/carmm] Assertion for surface-adsorbate distance in bond_length_scan (PR #122)

OK - does that introduce a minimum dependence from us on the ASE version number? This is something we need to consider anyway at some point for the calculator, I guess... ____ From: Igor Kowalec @.> Sent: 09 August 2023 13:52 To: logsdail/carmm @.> Cc: Andrew Logsdail @.>; Review requested @.> Subject: Re: [logsdail/carmm] Assertion for surface-adsorbate distance in bond_length_scan (PR #122https://github.com/logsdail/carmm/pull/122) External email to Cardiff University - Take care when replying/opening attachments or links. Nid ebost mewnol o Brifysgol Caerdydd yw hwn - Cymerwch ofal wrth ateb/agor atodiadau neu ddolenni. @ikowalechttps://github.com/ikowalec commented on this pull request. ____ In carmm/build/neb/bond_length_scan.py<#122 (comment)https://github.com/logsdail/carmm/pull/122#discussion_r1288435817>: @@ -81,12 +86,12 @@ def take_second(n): for i in range(1, n_steps+1): # operate on a deepcopy for intended functionality - atoms = copy.deepcopy(atoms) + atoms = atoms.copy() Yes, in previous versions atoms.copy() was providing a shallow copy and now it is a completely new instance, so using copy library can be avoided — Reply to this email directly, view it on GitHub<#122 (comment)https://github.com/logsdail/carmm/pull/122#discussion_r1288435817>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFMK4MYKIXEENPWLMXRFRPLXUOB2BANCNFSM6AAAAAA3JYVWPQ. You are receiving this because your review was requested.Message ID: @.***>

Re: codebase, the deepcopy can copy the calculator, but it is generally frowned upon as per: https://gitlab.com/gpatom/ase-gpatom/-/issues/8 For copying Atoms objects we should be using the dedicated Atoms.copy() function. It drops the calculator altogether, so a new one might have to be generated if required.

— Reply to this email directly, view it on GitHubhttps://github.com/logsdail/carmm/pull/122#issuecomment-1671338402, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFMK4M2FUTWBF57EXVEEEOLXUOGWFANCNFSM6AAAAAA3JYVWPQ. You are receiving this because your review was requested.Message ID: @.***>

logsdail commented 1 year ago

Maybe note in the code as a TODO? Just so we have the edge case noted in case it recurs - we shouldn't just leave known issues undocumented.

From: Igor Kowalec @.> Sent: 09 August 2023 14:39 To: logsdail/carmm @.> Cc: Andrew Logsdail @.>; Review requested @.> Subject: Re: [logsdail/carmm] Assertion for surface-adsorbate distance in bond_length_scan (PR #122)

External email to Cardiff University - Take care when replying/opening attachments or links. Nid ebost mewnol o Brifysgol Caerdydd yw hwn - Cymerwch ofal wrth ateb/agor atodiadau neu ddolenni.

@ikowalec commented on this pull request.

In carmm/build/neb/bond_length_scan.pyhttps://github.com/logsdail/carmm/pull/122#discussion_r1288500521:

 # retrieve z-coordinate for z_bias

if z_bias:

  • Define a list of atom chemical symbols and their count

  • Take the z coordinate of the most abundant element in the surface slab

  • chem_symbol_count = [[x, atoms.get_chemical_symbols().count(x)] for x in set(atoms.get_chemical_symbols())]

  • Sort the list by the count

  • chem_symbol_count.sort(key=lambda k: k[1])
  • surf_z_list = [atom.z for atom in atoms if atom.symbol == chem_symbol_count[-1][0] and atom.tag > 0]

I removed the use of tags and replaced them with avoiding z-coordinates of atoms being moved. The only edge case I can think of is e.g. metal oxides with more oxygen than metal and large adsorbates containing oxygen, where not all oxygens are being moved. Then potentially the dynamically applied z_bias might be incorrect, but can always be replaced by a float.

— Reply to this email directly, view it on GitHubhttps://github.com/logsdail/carmm/pull/122#discussion_r1288500521, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFMK4MY2F432QIYWK6TGZA3XUOHJPANCNFSM6AAAAAA3JYVWPQ. You are receiving this because your review was requested.Message ID: @.***>