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

Improve parameter checks for flowrate_filter #109

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

schmoelder
Copy link
Contributor

This is an initial draft to improve parameter checks.

In the given example, the FLOWRATE_FILTER accidentally did not have enough entries. Because its length was still > 0, CADET assumed it was a section depenent parameter. However, when compiled with g++, the CADET would exceed the bounds of the array and only (sometimes) crash much later. On Windows, it crashes immediately when exceeding the bounds of the array.

By calling getSectionDependentScalar(_flowRateFilter, secIdx) instead of indexing directly, it also crashes on Linux.

A more elegant approach would be to check the length of section dependent parameters before even starting the simulation. This way, the user knows directly that the configuration is invalid. @sleweke is this done somewhere in CADET for other section dependent parameters? I couldn't find anything.

Example

Partially fixes #28

@schmoelder schmoelder force-pushed the fix/parameter_checks branch from 3cb1519 to 11ac434 Compare January 18, 2022 17:02
@sleweke-bayer
Copy link
Contributor

You propose to change CSTRModel::notifyDiscontinuousSectionTransition() to

void CSTRModel::notifyDiscontinuousSectionTransition(double t, unsigned int secIdx, const ConstSimulationState& simState, const AdJacobianParams& adJac)
{
	if (_flowRateFilter.size() > 1)
	{
		_curFlowRateFilter = getSectionDependentScalar(_flowRateFilter, secIdx);
	}
	else if (_flowRateFilter.size() == 1)
	{
		_curFlowRateFilter = _flowRateFilter[0];
	}
}

You can further simplify, because this is equivalent:

void CSTRModel::notifyDiscontinuousSectionTransition(double t, unsigned int secIdx, const ConstSimulationState& simState, const AdJacobianParams& adJac)
{
	_curFlowRateFilter = getSectionDependentScalar(_flowRateFilter, secIdx);
}

@sleweke-bayer
Copy link
Contributor

is this done somewhere in CADET for other section dependent parameters? I couldn't find anything.

I don't know either. Would need to scan through the code as well.

@schmoelder schmoelder force-pushed the fix/parameter_checks branch from cbe8b6e to 895538d Compare April 4, 2022 08:42
@schmoelder schmoelder force-pushed the fix/parameter_checks branch from 895538d to 0f7defb Compare April 4, 2022 08:43
cadet::IModelSystem* model = _builder->createSystem(pp);

// Hand model over to simulator
_sim->initializeModel(*model);
_sim->setSectionTimes(secTimes, secCont);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is deleted, section times will not be propagated into the models. But, this is required for the inlet profiles to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Improve parameter checks and error messages before running a simulation
3 participants