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

Add new functions/methods to featurize module #330

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

naik-aakash
Copy link
Collaborator

@naik-aakash naik-aakash commented Sep 10, 2024

Closes #329

Changes

  • Add ultiliy functions to compute reduced masses, electronegativity and sort dict by values
  • Add the possibility to modify default lobsterpy kwargs in featurizers
  • Add ICOXX featurizer with BWDF functionality
  • Add tests

@naik-aakash naik-aakash marked this pull request as draft September 10, 2024 15:44
@naik-aakash naik-aakash self-assigned this Sep 10, 2024
@naik-aakash naik-aakash added the enhancement New feature or request label Sep 10, 2024
@naik-aakash
Copy link
Collaborator Author

Hi @JaGeo, there seems to be some issue with coverage artificats not being generated and uploaded in CI, I have not been able to pinpoint the cause yet. Will check again in detail tomorrow. Leaving this as a comment only for my reference here.

@naik-aakash
Copy link
Collaborator Author

Could be simply bug associated with a new release of pytest today. Will have to check downgrading it

@JaGeo
Copy link
Owner

JaGeo commented Sep 10, 2024

@naik-aakash sure. Don't worry!

@naik-aakash
Copy link
Collaborator Author

@naik-aakash sure. Don't worry!

issue was with upload artificats update ignoring hidden files from being uploaded. It should be fixed now in PR #331

@naik-aakash naik-aakash marked this pull request as ready for review September 16, 2024 12:39
@naik-aakash naik-aakash changed the title [WIP] add new functions/methods to featurize module Add new functions/methods to featurize module Sep 16, 2024
@naik-aakash naik-aakash marked this pull request as draft September 17, 2024 09:08
@naik-aakash
Copy link
Collaborator Author

Hi @JaGeo , posting here results of some tests I did to verify the implementation for BWDF

Note: In all the tests, a supercell of 2x2x2 is constructed

ZnSb Structure : mp-753

bwdf_Sb-Sb_ZnSb
bwdf_Sb-Zn_ZnSb
bwdf_summed_ZnSb
bwdf_Zn-Zn_ZnSb

NaCl

bwdf_Cl-Cl_NaCl
bwdf_Cl-Na_NaCl
bwdf_Na-Na_NaCl
bwdf_summed_NaCl

Si3N4 : mp-11607

bwdf_N-N_Si3N4
bwdf_N-Si_Si3N4
bwdf_Si-Si_Si3N4
bwdf_summed_Si3N4

AlN : mp-661

bwdf_Al-Al_AlN
bwdf_Al-N_AlN
bwdf_N-N_AlN
bwdf_summed_AlN

@naik-aakash
Copy link
Collaborator Author

Just for NaCl and AlN, I do not have exactly overlapping peaks. But seems to work in other cases. Can't seem to figure out what could be the reason.

@naik-aakash naik-aakash marked this pull request as ready for review October 28, 2024 15:04
@JaGeo
Copy link
Owner

JaGeo commented Oct 28, 2024

@naik-aakash can you provide the structures for both AlN cases and both NaCl cases here? And icohps for all Al-Al and cl-cl.interactions?

@naik-aakash
Copy link
Collaborator Author

@naik-aakash can you provide the structures for both AlN cases and both NaCl cases here? And icohps for all Al-Al and cl-cl.interactions?

cannot upload compressed files here, will share the calc files on email.

@naik-aakash
Copy link
Collaborator Author

Hi @JaGeo , Below you can find the outputs for the same structures with updated implementation, which now works fine for primitive and supercells

NaCl

bwdf_Cl-Cl_NaCl_new
bwdf_Cl-Na_NaCl_new
bwdf_Na-Na_NaCl_new
bwdf_summed_NaCl_new

AlN

bwdf_Al-Al_AlN_new
bwdf_Al-N_AlN_new
bwdf_N-N_AlN_new
bwdf_summed_AlN_new

Si3N4

bwdf_N-N_Si3N4_new
bwdf_N-Si_Si3N4_new
bwdf_Si-Si_Si3N4_new
bwdf_summed_Si3N4_new

ZnSb

bwdf_Sb-Sb_ZnSb_new
bwdf_Sb-Zn_ZnSb_new
bwdf_summed_ZnSb_new
bwdf_Zn-Zn_ZnSb_new

@JaGeo
Copy link
Owner

JaGeo commented Oct 30, 2024

Very good. I think you can test some more cases and see if your implementation can cover all of those

@QuantumChemist
Copy link
Contributor

Hi @JaGeo , posting here results of some tests I did to verify the implementation for BWDF

Note: In all the tests, a supercell of 2x2x2 is constructed

ZnSb Structure : mp-753

bwdf_Sb-Sb_ZnSb bwdf_Sb-Zn_ZnSb bwdf_summed_ZnSb bwdf_Zn-Zn_ZnSb

NaCl

bwdf_Cl-Cl_NaCl bwdf_Cl-Na_NaCl bwdf_Na-Na_NaCl bwdf_summed_NaCl

Si3N4 : mp-11607

bwdf_N-N_Si3N4 bwdf_N-Si_Si3N4 bwdf_Si-Si_Si3N4 bwdf_summed_Si3N4

AlN : mp-661

bwdf_Al-Al_AlN bwdf_Al-N_AlN bwdf_N-N_AlN bwdf_summed_AlN

Looks like you only checked cubic and tetragonal crystal symmetries so far. You could try some less symmetry structures as well, like monoclinic, triclinic. And also hexagonal and orthorhombic.

@QuantumChemist
Copy link
Contributor

Oh, I replied to the wrong comment

@naik-aakash
Copy link
Collaborator Author

Hi @JaGeo and @QuantumChemist , I did some more tests on different kinds of structures for supercell and primitive comparisons. Just posting here some results of summed BWDF I got from these comparisons for this structures. It seems to work in these cases as well.

orthorhombic : mp-10381

ortho_bwdf_summed_K2CuSb_new

monoclinic : mp-10408

mono_bwdf_summed_K2CN2_new

hexagonal : mp-10614

hexa_bwdf_summed_SrLiP_new

triclinic : mp-14983

tri_bwdf_summed_Si4P4Ru_new

@JaGeo
Copy link
Owner

JaGeo commented Nov 5, 2024

Good, I would recommend testing your implementation on your complete dataset and check if there are any failures (does the algorithm find all ICOHPs in the ICOHP list?)

@naik-aakash
Copy link
Collaborator Author

Good, I would recommend testing your implementation on your complete dataset and check if there are any failures (does the algorithm find all ICOHPs in the ICOHP list?)

Will check this soon 😄

@QuantumChemist
Copy link
Contributor

Hi @JaGeo and @QuantumChemist , I did some more tests on different kinds of structures for supercell and primitive comparisons. Just posting here some results of summed BWDF I got from these comparisons for this structures. It seems to work in these cases as well.

orthorhombic : mp-10381

ortho_bwdf_summed_K2CuSb_new

monoclinic : mp-10408

mono_bwdf_summed_K2CN2_new

hexagonal : mp-10614

hexa_bwdf_summed_SrLiP_new

triclinic : mp-14983

tri_bwdf_summed_Si4P4Ru_new

Fantastic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update featurizer module with more options
3 participants