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

Fix CSTR interface documentation and ensure backwards compatibility #286

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

jbreue16
Copy link
Contributor

In #235 we changed the field "INIT_VOLUME" to "INIT_LIQUID_VOLUME" but forgot to update the documentation.
Also, Ive added backwards compatibility even though volume in the new CSTR refers to liquid volume only, since this unit operation is often used with constant volume and without a solid phase.
We can delete the backwards compatibility for the new interface version, i.e. for cadet-core v.6.0.0

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.

LGTM. If I could chose, I would probably have used INIT_VOLUME_LIQUID/SOLID but that's not really a deal breaker and just personal preference.

Edit: @jbreue16 You mentioned that we're adding more tests for the CSTR to better catch this in the future, right?

@jbreue16
Copy link
Contributor Author

jbreue16 commented Sep 10, 2024

Edit: @jbreue16 You mentioned that we're adding more tests for the CSTR to better catch this in the future, right?

Yes, theres issue #125 and the meta issue #254 where many open ends for our testing suite are tracked, including
Add test where CSTR runs empty (but not negative) wrt liquid volume : Solid phase should stay constant
Add CSTR consistent initialization test with vLiquid = 0

@jbreue16 jbreue16 merged commit 4fb2156 into master Sep 10, 2024
4 checks passed
@jbreue16 jbreue16 deleted the fix/CSTR branch September 10, 2024 10:50
@github-actions github-actions bot locked and limited conversation to collaborators Sep 10, 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.

2 participants