Extend lobsterenv for coop/cobi #3050
Conversation
|
Hi @JaGeo , if you have any more suggestions / changes let me know |
naik-aakash
changed the title
pymatgen/io/lobster/lobsterenv.py
Outdated
| adapt_extremum_to_add_cond: should the extrumum be adapted to the additional condition | ||
| additional_condition: additional condition to determine which bonds are relevant | ||
| Returns: [-100000, min(max_icohp*0.15,-0.1)] | ||
| Returns: [-100000, min(max_icohp*0.15,-0.1)] / [max_icohp*0.15,100000] |
There was a problem hiding this comment.
Could we make this -0.1 hard limit a parameter and allow to turn it off and make it adjustable? This is my fault
Have you added tests that this part is working correctly?
There was a problem hiding this comment.
I will be adding more tests soon. At the moment this is not checked. I will remove the hard code for minimum limits
There was a problem hiding this comment.
Maybe, we can find a more elegant solution that also works for different units. float('inf') maybe?
There was a problem hiding this comment.
Now limits returned uses float('inf') instead of hardcoded numbers
pymatgen/io/lobster/lobsterenv.py
Outdated
| extremum_based = max(list_icohps) * percentage | ||
|
|
||
| if not self.are_coops and not self.are_cobis: | ||
| max_here = min(extremum_based, -0.1) |
There was a problem hiding this comment.
Same here. It would be good if we could change this parameter.
There was a problem hiding this comment.
added noise_cutoff parameter
…pymatgen into cobi_coop_lobsterenv_extend sync with remote
| self.add_additional_data_sg = add_additional_data_sg | ||
| self.filename_blist_sg1 = filename_blist_sg1 | ||
| self.filename_blist_sg2 = filename_blist_sg2 | ||
| self.noise_cutoff = noise_cutoff |
There was a problem hiding this comment.
Documentation for the noise_cutoff needs to be added :).
There was a problem hiding this comment.
btw, could you add your one example case from the db as a test, where the noise-cutoff really made a different?
There was a problem hiding this comment.
Yeah will do add it 😄
There was a problem hiding this comment.
Hi @JaGeo , I have added a test case now, that also checks the impact of the noise_cutoff parameter. The example structure is from our database (mp-632319), but apparently, our ICOHP cutoff of 10 %, actually already does not consider Cs-H bonds. So had to reduce the ICOHP cutoff further in tests, to check if the parameter works as intended.
pymatgen/io/lobster/lobsterenv.py
Outdated
| adapt_extremum_to_add_cond: should the extrumum be adapted to the additional condition | ||
| additional_condition: additional condition to determine which bonds are relevant | ||
| Returns: [-100000, min(max_icohp*0.15,-0.1)] | ||
| Returns: [-inf, min(max_icohp*0.15,noise_cutoff)] / [max(max_icohp*0.15, noise_cutoff),inf] |
There was a problem hiding this comment.
I think you mixed up plus and minus here. It should be -0.1. Currently, it's +0.1
There was a problem hiding this comment.
Yeah , will fix this
There was a problem hiding this comment.
Fixed the error in doc-string
There was a problem hiding this comment.
Maybe put (ICOHP) and (ICOOP, ICOBI) in brackets? Not sure people will directly get the "/". I would also replace "/" with or.
There was a problem hiding this comment.
Maybe, we should also say max_abs_icohp. Otherwise, very good :).
There was a problem hiding this comment.
changed doc-string to strongest_icohp as per your suggestion 😃
|
@naik-aakash , very nice! I have added my tiny suggestion on the documentation and naming (which was initially, of couse, my fault ;))! |
…pymatgen into cobi_coop_lobsterenv_extend sync precommit auto fixes
|
Ready to be merged from my side! @naik-aakash @janosh , this is ready to be reviewed! :) |
There was a problem hiding this comment.
Looking good overall. A few suggestions:
- Best to remove the [WIP] from PR title when review-ready.
- Do we really need the new 90 KB
test_files/cohp/environments/ICOBILIST.lobster.mp_470.gz? How does it differ from existing test files? CanICOBILIST.lobster.NaCl.gznot be re-used?
pymatgen/io/lobster/lobsterenv.py
Outdated
| if not self.are_coops and not self.are_cobis: | ||
| extremum_based = min(list_icohps) * percentage | ||
| else: | ||
| extremum_based = max(list_icohps) * percentage |
There was a problem hiding this comment.
This code is repeated 5 times. Maybe turn into a function. Could definitely be made more concise:
| if not self.are_coops and not self.are_cobis: | |
| extremum_based = min(list_icohps) * percentage | |
| else: | |
| extremum_based = max(list_icohps) * percentage | |
| which_extr = min if not self.are_coops and not self.are_cobis else max | |
| extremum_based = which_extr(list_icohps) * percentage |
There was a problem hiding this comment.
This code is repeated 5 times. Maybe turn into a function. Could definitely be made more concise:
Thanks for this suggestion . I have implemened a method for this based on your suggestion.
naik-aakash
changed the title
I included the |
|
Hi @janosh , I think this should be ready to be merged if all seems fine to you. Also, it would be great, it the contributors list could be updated. I have filled out the form on the website several times, but so far it does not seem to work for me |
|
Sorry for the delay @naik-aakash, this seems to have gotten lost in my inbox. |
janosh
added
enhancement
No worries. I suspected this happened, as you are working constantly on so many tasks. It is totally fine and thanks again 😃 |
Additions