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 of mixing rules #333

Merged
merged 49 commits into from
Aug 1, 2024
Merged

Fix of mixing rules #333

merged 49 commits into from
Aug 1, 2024

Conversation

HomesGH
Copy link
Contributor

@HomesGH HomesGH commented Jul 26, 2024

Description

The mixing rules are kind of messed up for now, see issues:

#73

  • misleading warning "This can happen because the xml-input doesn't support these yet!"
  • neededCoeffs check should be multiplied by two (if(dmixcoeff.size() < neededCoeffs){) and this should probably be an ==
  • duplication between Domain::_mixcoeff and EnsembleBase::_mixingrules
  • special mixing rules with xi and eta are not symmetric, but Comp2Param::initialize is symmetric? --> the classical modified Lorentz-Berthelot rule (the present one) is symmetric. No other rule is implemented so far and probably will never be...
  • appending values at the end of _mixcoeff in Simulation.cpp:250 error prone
  • initialization is dependent on the order in which they appear in the file and the passed componentid-s have no effect ?

#74

  • Initialization of special mixing rules in XML is order dependent and ignores the cid1 and cid2 values

#75

  • Comp2Param::initialize applies special mixing rules xi and eta symmetrically --> introduced generic functions, but the present classical LB is symmetric

#331

  • cids specified in <mixing> have no effect

There was already an attempt #76 for a fix but it was not merged but closed.

It would be elegant to use a lambda function for the application of the (here and here) rule to make it more generic. Unfortunately, I wasn't able to make it work due to the scopes here. Help is appreciated 😉
The open points above would also be solved by applying the LB-specific lambda function in the Comp2Param.

Done

The central change is introducing a map of Mixingrules as storage and also USE them later in the Comp2Params
src/molecules/mixingrules/MixingRuleBase.h Fixed Show resolved Hide resolved
src/molecules/mixingrules/MixingRuleBase.h Fixed Show fixed Hide fixed
src/molecules/mixingrules/MixingRuleBase.h Fixed Show fixed Hide fixed
src/molecules/mixingrules/LorentzBerthelot.h Fixed Show fixed Hide fixed
src/molecules/Comp2Param.cpp Fixed Show fixed Hide fixed
src/molecules/Comp2Param.cpp Fixed Show fixed Hide fixed
@FG-TUM
Copy link
Member

FG-TUM commented Jul 26, 2024

There was already an attempt #76 for a fix but it was not merged but closed.

It would be elegant to use a lambda function for the application of the (here and here) rule to make it more generic. Unfortunately, I wasn't able to make it work due to the scopes here. Help is appreciated 😉

Sorry I don't fully understand what is the problem here. Can you clarify?

@HomesGH
Copy link
Contributor Author

HomesGH commented Jul 26, 2024

There was already an attempt #76 for a fix but it was not merged but closed.
It would be elegant to use a lambda function for the application of the (here and here) rule to make it more generic. Unfortunately, I wasn't able to make it work due to the scopes here. Help is appreciated 😉

Sorry I don't fully understand what is the problem here. Can you clarify?

For now, the LB rule is used in the general calculations in Comp2Params:

epsilon24 = 24. * xi * sqrt(epsi * epsj); // This is also LB -> better generic (lambda) function
sigma2 = eta * .5 * (sigi + sigj); // This is also LB -> better generic (lambda) function

Since other rules might be introduced later, this is not ideal and it would be better to do something like (note the lambda expressions):

if (mixingrule->getType() == "LB") {
				eta = mixingrule->getParameters().at(0);
				xi  = mixingrule->getParameters().at(1);
				auto mixingSigma = [&](double epsi, double epsj) { return xi * sqrt(epsi * epsj); }
				auto mixingEpsilon = [&](double epsi, double epsj) { return xi * sqrt(epsi * epsj); }
// #ifndef NDEBUG
				Log::global_log->info() << "Mixing : cid+1(compi)=" << compi+1 << " <--> cid+1(compj)=" << compj+1 << ": xi=" << xi << ", eta=" << eta << std::endl;
// #endif
			} else {
				Log::global_log->error() << "Mixing: Only LB rule supported" << std::endl;
				Simulation::exit(1);
			}

and later in the calculations

epsilon24 = 24. * mixingEpsilon (epsi, epsj);
sigma2 = .5 * mixingSigma (sigi, sigj);

instead of the current (LB-specific) implementation.

But with this, the lambda expressions are just defined in the scope of the if statement.

@FG-TUM
Copy link
Member

FG-TUM commented Jul 26, 2024

Ah ok so you want to store functions in the variables and later use them to get the actual mixing coefficients, correct?

You could do something like this:

std::function<double(double, double)> mixingEpsilon;
std::function<double(double, double)> mixingSigma;

if (mixingrule->getType() == "LB") {
    // ...
    mixingSigma = [=](double epsi, double epsj) { return xi * sqrt(epsi * epsj); };
    mixingEpsilon = [=](double epsi, double epsj) { return xi * sqrt(epsi * epsj); };
}

Note the use of taking xi and epsi by copy ([=]) since the here-referenced variables probably won't exist anymore when you want to invoke the lambdas.

I've almost never used the std::function interface. Therefore, I have no idea about its performance implications. Where exactly would you want to invoke the functions?

@HomesGH
Copy link
Contributor Author

HomesGH commented Jul 26, 2024

I've almost never used the std::function interface. Therefore, I have no idea about its performance implications. Where exactly would you want to invoke the functions?

Thank you! I have used std::function now. The code is used during the initialization of the comp2params and, therefore, just called once or twice at the beginning of the simulation.

@FG-TUM FG-TUM removed their request for review July 29, 2024 10:24
@HomesGH HomesGH requested a review from FG-TUM July 29, 2024 13:33
@HomesGH HomesGH added the bug Something isn't working label Jul 29, 2024
Copy link
Member

@FG-TUM FG-TUM left a comment

Choose a reason for hiding this comment

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

Why did you remove the cpp files? Usually the headers should only contain declarations and the implementations go into the cpps

src/molecules/mixingrules/MixingRuleBase.h Outdated Show resolved Hide resolved
src/molecules/mixingrules/MixingRuleBase.h Outdated Show resolved Hide resolved
src/molecules/mixingrules/MixingRuleBase.h Outdated Show resolved Hide resolved
src/molecules/mixingrules/MixingRuleBase.h Outdated Show resolved Hide resolved
src/molecules/mixingrules/MixingRuleBase.h Outdated Show resolved Hide resolved
src/ensemble/EnsembleBase.cpp Outdated Show resolved Hide resolved
src/ensemble/EnsembleBase.cpp Outdated Show resolved Hide resolved
src/ensemble/EnsembleBase.cpp Outdated Show resolved Hide resolved
src/Domain.cpp Outdated Show resolved Hide resolved
src/Domain.cpp Outdated Show resolved Hide resolved
src/Simulation.cpp Dismissed Show dismissed Hide dismissed
@FG-TUM FG-TUM changed the base branch from master to LustigFormalism July 30, 2024 14:00
@FG-TUM FG-TUM changed the base branch from LustigFormalism to master July 30, 2024 14:00
@HomesGH HomesGH requested a review from FG-TUM July 30, 2024 16:13
Copy link
Member

@FG-TUM FG-TUM left a comment

Choose a reason for hiding this comment

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

Just some unnecessary copies then we are good to merge!

src/Domain.cpp Outdated Show resolved Hide resolved
src/molecules/mixingrules/MixingRuleBase.h Outdated Show resolved Hide resolved
@HomesGH HomesGH requested a review from FG-TUM August 1, 2024 04:57
@FG-TUM FG-TUM merged commit ced676e into master Aug 1, 2024
52 checks passed
@FG-TUM FG-TUM deleted the mixLog branch August 1, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants