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

Update MCT Documentation #230

Merged
merged 5 commits into from
Jul 3, 2024
Merged

Update MCT Documentation #230

merged 5 commits into from
Jul 3, 2024

Conversation

hannahlanzrath
Copy link
Collaborator

@hannahlanzrath hannahlanzrath commented Jun 27, 2024

Fixes Issue #223

Throughout this PR we've also found and fixed some issues with the radial flow documentation, and updated the MCT interface so that NCHANNEL is not a discretization but model parameter. These changes will be put in separate commits.

Copy link

github-actions bot commented Jun 27, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@hannahlanzrath
Copy link
Collaborator Author

hannahlanzrath commented Jun 27, 2024

  • The CADET maintainers know my real name.

  • The CADET maintainers know my current employer.

@hannahlanzrath
Copy link
Collaborator Author

I have read the CLA Document and I hereby sign the CLA.

@hannahlanzrath hannahlanzrath self-assigned this Jun 27, 2024
@hannahlanzrath
Copy link
Collaborator Author

hannahlanzrath commented Jun 27, 2024

Mind that

Fix spelling Mistake within Radial Flow Model (found by accident)
3c3dfce

was not the objective of this PR and maybe should be handled separately when merging.

@jbreue16

@jbreue16
Copy link
Contributor

  • The CADET maintainers know my real name.

  • The CADET maintainers know my current employer.

@jbreue16
Copy link
Contributor

I have read the CLA Document and I hereby sign the CLA.

Copy link
Contributor

@schmoelder schmoelder left a comment

Choose a reason for hiding this comment

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

Please also provide links between modeling/interface like with the other models.

@cadet cadet deleted a comment from jbreue16 Jun 27, 2024
@jbreue16
Copy link
Contributor

Ive added the general MCT equations to the documentation.
Since the model is rather unknown compared to our chromatography models, i think its fine having a description before we state the equations. The tracer transport should go into an individual subsection (where i think we should also cite a corresponding paper once its out).
@hannahlanzrath feel free to adjust my changes, I was not 100% sure about where every paragraph should go.

@hannahlanzrath
Copy link
Collaborator Author

So I had a look again at the Radial Flow Documentation that is currently up live on

https://cadet.github.io/master/interface/unit_operations/radial_flow_models.html

and spotted a few more Issues.

image

I will correct them in this PR but it should probably be squashed separately during the merge.

@jbreue16 @schmoelder

@hannahlanzrath hannahlanzrath changed the title Update / Fix MCT Documentation Update / Fix MCT Documentation (+ Radial Flow Documentation) Jun 28, 2024
@jbreue16
Copy link
Contributor

Theres actually another PR #193 in which we wanted to go through and fix the documentation, including radial flow.. glad you found and fixed those but no need to delve deeper into this here. I agree on making two separate master commits from this PR

@jbreue16 jbreue16 mentioned this pull request Jun 28, 2024
4 tasks
@hannahlanzrath
Copy link
Collaborator Author

Ah sorry, I searched for a PR with the Radial Flow Doc and couldn't find one. Fixed the issues I mentioned an will just leave it at is now.

@hannahlanzrath
Copy link
Collaborator Author

Okay, with that last commit from my side this is ready to merge. Thanks a lot for contributing the equations!

@jbreue16 jbreue16 requested a review from schmoelder June 28, 2024 08:56
@jbreue16 jbreue16 changed the title Update / Fix MCT Documentation (+ Radial Flow Documentation) Update MCT Documentation Jun 28, 2024
jbreue16 added a commit that referenced this pull request Jun 28, 2024
* Adjust MCT interface doc

* Add general MCT equations to modelling section

* Add Link to Modelling from MCT Interface

* Move tracer transport to own use-case subsection

-------

Co-authored-by: Jan Breuer <[email protected]>
@jbreue16
Copy link
Contributor

jbreue16 commented Jun 29, 2024

Should we explicitly mention the results of the forum discussion in the MCT documentation, i.e. that the exchange rates are used to calculate the exchange flow wrt the volume of the outflow/source channel?
Maybe even a second use-case chapter on how to replicate the linear binding LRM with essentially the content of the forum post?

@hannahlanzrath
Copy link
Collaborator Author

I think that's a good idea to highlight and deepen the understanding of the differences between the models.

@jbreue16
Copy link
Contributor

jbreue16 commented Jul 1, 2024

@schmoelder this PR is good to be merged from our side

* Adjust MCT interface doc

* Add general MCT equations to modelling section

* Add Link to Modelling from MCT Interface

* Move tracer transport to own use-case subsection

-------

Co-authored-by: Jan Breuer <[email protected]>
@jbreue16 jbreue16 force-pushed the documentationMCT branch from 9b9b50d to e48aaf9 Compare July 3, 2024 10:13
@jbreue16 jbreue16 self-requested a review July 3, 2024 10:26
@jbreue16 jbreue16 merged commit 2b0b71a into master Jul 3, 2024
4 checks passed
jbreue16 added a commit that referenced this pull request Jul 3, 2024
* Adjust MCT interface doc

* Add general MCT equations to modelling section

* Add Link to Modelling from MCT Interface

* Move tracer transport to own use-case subsection

-------

Co-authored-by: Jan Breuer <[email protected]>
@jbreue16 jbreue16 deleted the documentationMCT branch July 3, 2024 10:38
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants