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

Added terms for reactor type description #74

Merged
merged 14 commits into from
May 10, 2024
Merged

Conversation

HendrikBorgelt
Copy link
Member

Added terms for reactor typ description.

These terms where added:

reactor type
plug flow reactor
stirred tank reactor
tubular reactor
coiled tubular reactor
continously stirred tank reactor
discontinous stirred tank reactor

@dalito
Copy link
Member

dalito commented Apr 26, 2024

You do not need to create new PRs after every change (#72, #73). You could just update the PR by committing more changes to your fork. After changes made by the gh-action, you just need to retrieve the changes back to your fork (git pull). Then you can commit the updated / changed Excel file to the same branch where you created the PR from.

You may have troubles because you committed the changes to the main branch of your fork instead of to a feature branch. Working on main in a fork is in general not a good idea because your main branch will then diverge from the upstream main. Instead it is better to create a feature branch after forking and work on (commit to) the feature branch and create a pull request from the feature branch.

@HendrikBorgelt
Copy link
Member Author

Question for @dalito: Do i still need to change some thing, due to the errors made via the pullrequests/forks, is there still an error in the terms or are the terms just under a final supervison (and therefore might be accepted in the near future?). Sry for the troubles and thank you for the help. Due to the Github action which delets the Excel file, it can be quite confusing to know how one has to change the initiated pull request. Would it also be acceptable to just change the generated ".ttl" file of the pull request?

@dalito
Copy link
Member

dalito commented Apr 26, 2024

Yes changing turtle is possible. For you it should be fine. However, for beginners there is a high risk to produce an inconsistent set of rdf files which will not pass validation. I think (put am not 100% sure) that a new, updated Excel file is made available as job artifact on the job summary page also if only turtle files change (e.g. https://github.com/nfdi4cat/voc4cat/actions/runs/8828690448).

I will look at the definitions later today.

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.

@HendrikBorgelt can you clarify if you want to define concepts/terms to describe reactor types

  • as used in modelling resulting in e.g. this def for CSTR reactor with infinitely fast mixing and continuous reactant feed and continuous product removal
  • as found in reality resulting in e.g. this def for CSTR stirred reactor with continuous reactant feed and continuous product removal. The existing concept reactor type covers this case.

The definitions would be somewhat different and there may be 2 distinct concepts needed.

vocabularies/voc4cat/0007101.ttl Show resolved Hide resolved
vocabularies/voc4cat/0007106.ttl Show resolved Hide resolved
@dalito dalito changed the title Added terms for reactor typ description (2) Added terms for reactor type description May 2, 2024
@dalito
Copy link
Member

dalito commented May 10, 2024

@HendrikBorgelt - I added hierarchy for reactor types and improved definitions to align with the coming guidelines-updates by @nmoust. Please have a look if you are OK with the proposed changes.

@HendrikBorgelt
Copy link
Member Author

Looks fine for me!

@dalito dalito merged commit d0f2cd7 into nfdi4cat:main May 10, 2024
1 check passed
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.

2 participants