-
Notifications
You must be signed in to change notification settings - Fork 837
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
uncertainty quantification in energy corrections #1558
Changes from 180 commits
ab7cbda
fdf0906
c18d772
bdee700
6001f99
e305f4c
92f8e52
ba4e971
24b4c27
4c149fd
70722e3
15aea74
74ce229
7e8d5aa
17a2adb
133be7a
643ff2d
1e6ff0e
f95b8bb
e87d34b
fa49b7f
05e3032
5db95eb
1e1bc7a
3cd1422
3223ebc
cc69b92
56fb1dd
3e12705
7d19415
14b4f63
3aa1efe
8188ff0
77632a0
a0775df
53f7853
6ee95af
78f12fe
95c5123
4fa38f5
939034d
ffe2b34
ddd0d51
13553cf
d6d2bc1
5c69a2b
f50a520
2816eb1
f29adb4
c3c4f39
b833765
b672c36
1789ebd
a3f44eb
fbaf465
9c2a477
a516fde
ffc8b84
8943da4
c35061c
f3eaac8
636edb3
2c11691
154798e
ee9cf90
d069055
f5ee309
f2ffd13
77cd7cc
3d1e74a
d4d5dd3
db53f00
620b6d9
08882fd
ed0c063
efd08de
9e8bb64
d9d9fbb
f147017
46143dc
ea64c06
9defaeb
06a85dc
4ea3016
5d12048
996e146
ce13201
6a8c752
b13a8f2
b8cd491
1428f10
859fdcd
d1e7d4e
7292872
3f193f5
d173a35
43bbbfb
369580c
48c6424
9709d5c
f83e52b
12967ca
69e9083
c63c332
9eb3c0a
efd526a
282b7c8
f09e08c
e8c10d8
65b69d4
2e95bfa
e625a76
a3e432d
bd1bd59
73c7254
3f6575f
8393985
c075384
37a10bc
5bd45a8
945290a
10732f2
e896037
c87c9fb
809b874
3c72edc
e1516cd
425c5e8
40552ad
ef53aef
8753dc2
0216023
07061c4
2508dda
18542a4
3484924
b2a2c57
47d7d1b
d7a216a
7bbe5e8
5c0b7a9
a8bead5
b8132fb
2696396
040390b
878ddcf
2826bdb
f7e2e99
d988d69
7bbe344
0c53423
762dfe3
ee48c90
8b68a22
109ab90
a7c285a
368c1de
52ee7ad
0caf06a
85f965e
4b9afdf
45ac225
9f7342f
6a08d5c
e3d1ade
f50954f
897e83f
85adc8a
e776f35
9f7d357
e2a5e1e
3df58a2
13b9ead
5ecbf45
b3acdb4
b10ef57
acf911e
267082f
d242aca
75404e3
48a89f8
aeb342b
fd4b587
83cb3df
a5e8759
373c208
358922d
84ca376
8348a72
111d322
7ab454d
5af1277
32f7ee2
2fa1b77
878734b
99fe7a7
f5f048d
dfda9ef
298ac4f
2c97a94
19ea07f
9ae9d54
155d2a5
e44e47f
4c98554
21d3349
8ffd207
2cd8073
635414d
9556fe1
bb5543f
3467c57
c463246
255749c
c94963e
3f7f475
9e21a5e
9e75eaa
e03ad90
eac0756
f24317b
f83d344
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -603,7 +603,7 @@ def oxide_type(structure, relative_cutoff=1.1, return_nbonds=False): | |
|
||
def sulfide_type(structure): | ||
""" | ||
Determines if a structure is a sulfide/polysulfide | ||
Determines if a structure is a sulfide/polysulfide/sulfate | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand changes here were reverted, but reading over the code for this PR...does this actually return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the average electronegativity of the coordinating elements around S are greater than the electronegativity of S, then yes, "sulfate" is returned (see line 642) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's for the inner function though, line 652 suggests None is then returned? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh weird, hadn't noticed that. Can you think of any reason we shouldn't return 'sulfate' there? |
||
|
||
Args: | ||
structure (Structure): Input structure. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this library, but is it necessary here (considering we would have to add as a core requirement)? It looks like the functionality of
uncertainties
is not used in this class beyond book-keeping, which could be done by a named_tuple or otherwise. I'd err towards keeping it but just wanted to clarify this point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beyond book-keeping, we use
uncertainties
to calculate the propagated uncertainty when adding/subtracting uncertain quantities. It would be possible to code this ourselves (we had it that way in a previous iteration), but I feel its more robust to let the dedicated library handle it. Also, in some of our previous discussions I thought you were in favor of addinguncertainties
as a core requirement, because it will facilitate some other functionality we have in the pipeline? In any event, I'd prefer to keep unless you strongly oppose. I think the package is pretty lightweight.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to respond to this, yes I'm in favor of
uncertainties
via being in favor ofpint
, so I don't mind it getting added here, but mostly with a view of also addressing the unit question down the road. If this were the only place we'd use it I'd be more hesitant.