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

Add aggregation and breakage to crystallization module #241

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

jbreue16
Copy link
Contributor

@jbreue16 jbreue16 commented Jul 9, 2024

There are multiple branches with parts of the functionality that we want, i.e. sole aggregation, sole breakage, sole PBM and all of them combined. However, the code is not modular.
The respective branches are on @WFlynnZ fork of CADET.
We need to figure out a neat modularization and then incorporate the code from the aforementioned branches, which we will do on this branch here.

Todo:

  • rebase on master after PR Crystallization reaction module #112 is merged
  • unify PBM, aggregation and fragmentation codes
  • Put all parts in the same _bin loop
    • add abstraction layer PBMKernel
    • put pbm, fragmentation and aggregation in the same bin loop
    • update Jacobian accordingly
    • Figure out a more efficient containerization of the parameters? (see ReconstructionParams and JacobianParams, which have too many/unused fields depending on which reconstruction is used)
    • use bitflags to determine the crystallization mode
  • Update the test data:
    • Update cadet-verification to vary the reconstruction schme used in crystallization tests (currently only upwind is tested)
    • update corresponding json and h5 files to cadet-core test data
  • add tests for all crystallization modes
  • add documentation for aggregation and fragmentation
  • Can we improve performance by storing repeatedly calculated values?

@WFlynnZ
Copy link
Collaborator

WFlynnZ commented Oct 7, 2024

Things to consider when modularizing the current code @jbreue16 :

  1. Aggregation, Breakage and PBM (Population Balance with Mass Balance) should be able to be solved independently, and combined with two or three together.
  2. Because Aggregation and Breakage are mass-conserving, the mass balance equation (and also PBM) is not needed when solving them independently.
  3. When solving the PBM, two additional components are added: solute concentration is the first component and the solubility is the last component. These two components are absent in the Aggregation and Breakage code. When changing code, need to make sure the residual and Jacobian are also adapted.

Possible combinations are:

  1. Aggregation
  2. Breakage
  3. PBM
  4. Aggregation, Breakage
  5. Aggregation, PBM
  6. Breakage, PBM
  7. Aggregation, Breakage, PBM

Only 1, 2, and 4 don't have the two additional components.

@jbreue16 jbreue16 force-pushed the feature/crystallization_agg_brk branch from 7412f7d to c4cb521 Compare December 16, 2024 14:54
@jbreue16 jbreue16 force-pushed the feature/crystallization_agg_brk branch from dedd7d3 to d8a0c43 Compare December 19, 2024 15:07
@jbreue16
Copy link
Contributor Author

@WFlynnZ I found that the Jacobian tests fail, if we set the growth_scheme_order to 2, 3 or 4, which also happens for the code on master .. do you have an idea why that is, or is there a bug in the Jacobian?

@WFlynnZ
Copy link
Collaborator

WFlynnZ commented Jan 2, 2025

@jbreue16 I remember we troubleshooted this a while ago, if I recall correctly, there was an issue with the CSTR module, which was fixed? I checked the PBM Jacobians a number of times now,,, it should be hard to find more mistakes in these higher order schemes, maybe this is related to the tolerances? Some entries need large tolerances because the absolute values are big.

@jbreue16 jbreue16 force-pushed the feature/crystallization_agg_brk branch from d9c87ab to 2d68ec0 Compare January 21, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants