Add rescaling of reference scales in EchemdbEntry#115
Merged
DunklesArchipel merged 48 commits intoechemdb:mainfrom Jan 15, 2026
Merged
Add rescaling of reference scales in EchemdbEntry#115DunklesArchipel merged 48 commits intoechemdb:mainfrom
DunklesArchipel merged 48 commits intoechemdb:mainfrom
Conversation
linuxrider
approved these changes
Dec 12, 2025
|
|
||
|
|
||
| """ | ||
| field_name = field_name or "E" |
Contributor
There was a problem hiding this comment.
Is an arbitrary field_name necessary in the frame of echemdb?
Member
Author
There was a problem hiding this comment.
Sure, for example, you could have a potential given for a ring electrode or the counter electrode that you might want to shift as well.
Co-authored-by: Johannes Hermann <linuxrider@web.de>
DunklesArchipel
commented
Dec 16, 2025
DunklesArchipel
commented
Dec 16, 2025
DunklesArchipel
commented
Dec 16, 2025
DunklesArchipel
commented
Dec 17, 2025
DunklesArchipel
commented
Dec 17, 2025
linuxrider
approved these changes
Dec 22, 2025
Co-authored-by: Johannes Hermann <linuxrider@web.de>
6 tasks
DunklesArchipel
commented
Jan 15, 2026
Comment on lines
+168
to
+182
| if filename or outdir: | ||
| if filename is None: | ||
| filename = "reference_electrode.json" | ||
| if outdir: | ||
| os.makedirs(outdir, exist_ok=True) | ||
| filepath = os.path.join(outdir, filename) | ||
| else: | ||
| filepath = filename | ||
| with open(filepath, "w", encoding="utf-8") as f: | ||
| json.dump(self.data, f, ensure_ascii=False, indent=4) | ||
| # json.dump does not save files with a newline, which compromises the tests | ||
| # where the output files are compared to an expected json. | ||
| f.write("\n") | ||
|
|
||
| return None |
Member
Author
There was a problem hiding this comment.
I assume this can be omitted and leave it to the user how the data should be exported from json. For those not versed with JSON, we could include and example in the documentation.
saraedum
reviewed
Jan 15, 2026
| full_name="0.5 M mercury / mercurous sulfate electrode", | ||
| temperature_dependence=[ | ||
| { | ||
| "formula": "E = 0.63495 - 781.44E-6 * T - 426.89E-9 * T**2", |
Member
There was a problem hiding this comment.
I hope we don't expect to be able to parse this some day?
Member
Author
There was a problem hiding this comment.
We still have to decide how we should handle these cases. But that will be a matter of discussion in the future.
saraedum
reviewed
Jan 15, 2026
saraedum
reviewed
Jan 15, 2026
saraedum
reviewed
Jan 15, 2026
saraedum
reviewed
Jan 15, 2026
saraedum
reviewed
Jan 15, 2026
|
|
||
| resource.custom["MutableResource"] = df_resource | ||
|
|
||
| # update offset in the fields |
Member
There was a problem hiding this comment.
I can't make sense of the code that follows. It's extremely complicated to me. I get what you're trying to do: record an offset in the resource and update the existing one if there's already one. But it's really hard to follow.
saraedum
reviewed
Jan 15, 2026
saraedum
approved these changes
Jan 15, 2026
Member
saraedum
left a comment
saraedum
left a comment
There was a problem hiding this comment.
I'd try to simplify things a bit but in principle this looks totally fine to me.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
doc/news/.Fixes #101
Fixes #107
So far we have the following reference electrodes in the echemdb database for aqueous systems, which have to be accounted for:
['RHE', 'SHE', 'SCE', 'Ag/AgCl', 'Hg/HgO/0.1M NaOH']Ag/AgClmight pose a problem, since the concentration is not given.Hg/HgO/0.1M NaOHmust be implemented. As long as we do not deconvolute the names, we should for the few cases simply add the offset. Reference values can be found here for all kinds of electrolytes: https://pubs.acs.org/doi/10.1021/acscatal.2c05655unitpackage.entry.Entry