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

Electrochemistry terms michael goette #55

Merged
merged 22 commits into from
Feb 28, 2024

Conversation

RoteKekse
Copy link
Contributor

Hey i am still working on the terms, but since i have some questions and comments i created already a pull request.

i took definition so far mostly from here: https://echem101.gamry.com/electrochemical-terms/

when do i get ids assigned? only when the pull request is finished or do i have to set them by hand in the excel file?

further what is with qualities such as current, electric potential, current density etc. should we put them in?

@RoteKekse RoteKekse force-pushed the electrochemistry_terms_michael_goette branch from b7d589b to ffce27c Compare February 21, 2024 14:12
@dalito
Copy link
Member

dalito commented Feb 21, 2024

The current failure is due to a detail of #49 which takes a bit of time to fix. I'll look into it later. It is not caused by your xlsx file.

when do i get ids assigned? only when the pull request is finished or do i have to set them by hand in the excel file?

Because you control your range of IDs you could already use the IDs. In voc4cat they are only after merging of the PR,

i took definition so far mostly from

What is the copyright?

@dalito
Copy link
Member

dalito commented Feb 21, 2024

@RoteKekse I fixed the problem by updating the action to a new version of voc4cat-tool (0.8.2).

You accidentally committed a temporary office file that I removed (inbox-excel-vocabs/.~lock.voc4cat.xlsx#).

My updates re-started CI and it successfully converted your xlsx to turtle files and removed the xlsx file in an auto-commit (22a4558).

If you need xlsx again to further edit your terms you can get a new xlsx file from the workflow artifacts. Click on the green check-mark left of the commit hash (def449c), then "details", then go to the summary of the job, and download the artifact at the bottom. The artifact is a zip file with re-built xlsx, RDF/turtle, and HTML-documentation.

@RoteKekse RoteKekse force-pushed the electrochemistry_terms_michael_goette branch from 22a4558 to eeee46c Compare February 22, 2024 08:16
@RoteKekse
Copy link
Contributor Author

RoteKekse commented Feb 22, 2024

@dalito i updated from the source mainyour latest changes and then a git rebase. it seems like that the excel file is not in the artifacts anymore at least i dont find it. the zip` file doesnt contain an excel just a log file

but i think i can find the excel in an older commit, i think it might be better only delete it if the CI is run on the main branch in the main repo

@RoteKekse
Copy link
Contributor Author

Hey @dalito i took the description know from here. https://knowledge.electrochem.org/ed/dict.htm# I would used links but i think these resources are not persistent. I put in most terms i saw in our lab in electrocatalysis, let me know what you think.

@RoteKekse RoteKekse marked this pull request as ready for review February 23, 2024 10:42
@RoteKekse
Copy link
Contributor Author

@dalito i know there is likely a lot to discuss but this is what i am using for the electrochemistry in OER in nomad. Having term IDs for these things would be great and we would be happy to test it :).

@dalito
Copy link
Member

dalito commented Feb 23, 2024

OK. We will have a look and be not too picky about minor details (which could be improved in follow-up PRs).

@dalito
Copy link
Member

dalito commented Feb 23, 2024

The hierarchy is reversed: "final potential"(7217) has child (skos:narrower) "electric potential" (7219) - I looks like you consequently have it reversed (not just for these two concepts).

@nmoust
Copy link
Collaborator

nmoust commented Feb 23, 2024

@RoteKekse, thank you very much for your contribution. Please see below a few corrections needed, and some suggestions / comments to consider:

URI Concept Comment
voc4cat_0007202 working electrode Maybe add WE as an alternative label?
voc4cat_0007203 counter electrode Maybe add CE as an alternative label?
voc4cat_0007202 reference electrode Maybe add RE as an alternative label?
voc4cat_0007209 electrochemical impedance spectroscopy Can the definition be shortened? Maybe delete from: "Due to the small amplitude... to ... a single measurement". Also "abbreviated as" can be deleted.
voc4cat_0007223 electrochemical environment Typo in definition: elctrochemical.
voc4cat_0007228 number of cycles Typos in definition: measurment and an.
voc4cat_0007231 beaker Use becker or beker as alternative labels if used.
voc4cat_0007234 charge transport Maybe create a separate concept for electrochemical potential?
voc4cat_0007236 h cell Maybe use h-cell, H cell, and H-cell as alternative labels. Also maybe change H2 to hydrogen.
voc4cat_0007238 voltammogramm Remove extra space before Graphical.
voc4cat_0007241 initial frequency Change EIS to electrochemical impedance spectroscopy, and add a full stop.
voc4cat_0007242 final frequency Change EIS to electrochemical impedance spectroscopy, and add a full stop.
voc4cat_0007243 reversible hydrogen electrode Typo in definition: sated. Also remove Abbreviated as RHE.
voc4cat_0007244 concentration Add full stop at the end of the definition.
voc4cat_0007245 solvent Add full stop at the end of the definition.
voc4cat_0007248 rotating-disk electrode Maybe add rotating disk electrode as an alternative label.
voc4cat_0007249 electrochemical cell Maybe create separate concepts for galvanic cell and electrolytic cell?
voc4cat_0007251 open circuit voltage measurement Remove space between electro and chemical.
voc4cat_0007219 electric potential Typo in definition: point --> points.

@RoteKekse
Copy link
Contributor Author

The hierarchy is reversed: "final potential"(7217) has child (skos:narrower) "electric potential" (7219) - I looks like you consequently have it reversed (not just for these two concepts).

Hey @dalito thats true i intuitively recorded the parents because then you have at most one. I am sorry will correct it.

@RoteKekse
Copy link
Contributor Author

Thanks @nmoust for the feedback. I get from your feedback, that in principle it looks good?

@RoteKekse
Copy link
Contributor Author

I added the comments and reversed the is a relation. i think there are still some is arelation to be added but i am not 100% sure let me know what you think :)

@RoteKekse RoteKekse force-pushed the electrochemistry_terms_michael_goette branch from f6db001 to 5b9302f Compare February 26, 2024 13:13
@RoteKekse RoteKekse force-pushed the electrochemistry_terms_michael_goette branch from 7df9d29 to cbdac11 Compare February 26, 2024 14:02
@RoteKekse
Copy link
Contributor Author

i inclued some comments from @schumannj

Copy link
Member

@dalito dalito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RoteKekse Thanks for updates. A few points are left, see comments.

vocabularies/voc4cat/0007219.ttl Outdated Show resolved Hide resolved
vocabularies/voc4cat/0007231.ttl Show resolved Hide resolved
vocabularies/voc4cat/0007232.ttl Outdated Show resolved Hide resolved
vocabularies/voc4cat/0007236.ttl Outdated Show resolved Hide resolved
vocabularies/voc4cat/0007209.ttl Outdated Show resolved Hide resolved
vocabularies/voc4cat/0007219.ttl Outdated Show resolved Hide resolved
vocabularies/voc4cat/0007228.ttl Outdated Show resolved Hide resolved
vocabularies/voc4cat/0007233.ttl Outdated Show resolved Hide resolved
@RoteKekse RoteKekse force-pushed the electrochemistry_terms_michael_goette branch from 8a8ddd6 to d58743d Compare February 27, 2024 12:12
@RoteKekse RoteKekse force-pushed the electrochemistry_terms_michael_goette branch from 3701b4f to 7ba8686 Compare February 27, 2024 12:15
@RoteKekse RoteKekse force-pushed the electrochemistry_terms_michael_goette branch from b54366a to d37be69 Compare February 28, 2024 10:28
@RoteKekse
Copy link
Contributor Author

@dalito @nmoust is it ok to merge it for now?

@dalito
Copy link
Member

dalito commented Feb 28, 2024

Yes. Just let me know if you want to update the electrical potential definition or if I should do it directly in ttl.

@RoteKekse I pushed the update.

@dalito dalito merged commit c64d5d5 into nfdi4cat:main Feb 28, 2024
1 check passed
@dalito
Copy link
Member

dalito commented Feb 28, 2024

@RoteKekse - Thank you for the contribution and your perseverance in the review process!

@RoteKekse
Copy link
Contributor Author

Thank you for doing it. This is really valuable to us. :) we will include the iri in nomad in the upcoming weeks :)

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

Successfully merging this pull request may close these issues.

3 participants