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

@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.

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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