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

Move NBOUND parameter from discretization to unit_xxx/adsorption parameters group #32

Open
schmoelder opened this issue Mar 10, 2020 · 5 comments

Comments

@schmoelder
Copy link
Contributor

Moving the parameter NBOUND from the discretization group (where it is needed in the code) to the unit_operation parameters group makes more sense in the user interface.

See also #18

@Immudzen
Copy link
Contributor

I would be okay with reading it from either location but I am against just moving it since you break all existing simulations and gain nothing in the process.

@schmoelder
Copy link
Contributor Author

I understand your point. And for now being able to read it from both places seems reasonable to me.

For consistency's sake I still think it should be moved but maybe this can be done at some point in the future when other breaking changes are also introduced.

Just for some background, in CADET-Process the configuration of discretization parameters is separate from the model parameters. Hence it leads to some inconveniences when finally setting up the h5 file. Nothing too dramatic, just has always bothered me. ;-)

@schmoelder
Copy link
Contributor Author

Actually, it should probably be moved to the adsorption parameters group since it sets the expected length of the isotherm parameters.

@schmoelder schmoelder changed the title Move NBOUND parameter from discretization to unit_operation parameters group Move NBOUND parameter from discretization to unit_xxx/adsorption parameters group Nov 2, 2020
@lieres
Copy link
Member

lieres commented Nov 3, 2020

I agree with Bill that we should be very careful with breaking changes unless absolutely neccessary.

@schmoelder
Copy link
Contributor Author

schmoelder commented Dec 18, 2020

I also found that the number of particle types NPARTYPE is set in the discretization branch.

While I agree that we should definitely not introduce breaking changes (either by implementing a file version converter or by simply looking at the old location if parameter is not found), the location of these parameters is somewhat unintuitive.
I would rather keep the physicochemical model separate from the numerical discretization. This is especially relevant since now the configuration of adsorption models and multiple particle types is different for the CSTR and the column models.

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

No branches or pull requests

3 participants