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 LOOCV config option #2153

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

Conversation

davidscn
Copy link
Member

@davidscn davidscn commented Dec 5, 2024

Main changes of this PR

Follow-up on #2059, which makes the change user-configurable and enables a consistent logging.

Motivation and additional information

I left out the parallel aggregation for PUM, as it would introduce unnecessary communication.

Author's checklist

  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I added a changelog file with make changelog if there are user-observable changes since the last release.

Please check the naming of the option, the logging and the default value.

Reviewers' checklist

  • Does the changelog entry make sense? Is it formatted correctly?
  • Do you understand the code changes?

@fsimonis fsimonis marked this pull request as draft December 9, 2024 12:46
Copy link
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

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

Still a few hacks in here, but it is still a draft 😄

Could you please give an example output for some tutorial?

Currently, we can only switch it on or off and default is off. Maybe we want sth in between in the end(?)

@@ -75,7 +75,7 @@ struct BackendSelector {
// Specialization for the RBF Eigen backend
template <typename RBF>
struct BackendSelector<RBFBackend::Eigen, RBF> {
typedef mapping::RadialBasisFctMapping<RadialBasisFctSolver<RBF>> type;
typedef mapping::RadialBasisFctMapping<RadialBasisFctSolver<RBF>, bool> type;
Copy link
Member

Choose a reason for hiding this comment

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

Brief docs what the bool is.

@@ -217,6 +217,8 @@ MappingConfiguration::MappingConfiguration(
auto attrPolynomial = makeXMLAttribute(ATTR_POLYNOMIAL, POLYNOMIAL_SEPARATE)
.setDocumentation("Toggles use of the global polynomial")
.setOptions({POLYNOMIAL_ON, POLYNOMIAL_OFF, POLYNOMIAL_SEPARATE});
auto attrCrossValidation = makeXMLAttribute(ATTR_CROSS_VALIDATION, false)
.setDocumentation("Computes and logs a cross-validation error using the coupling data and the mapping matrix. This can be useful for tuning the basis function. Note that this might become computationally expensive.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.setDocumentation("Computes and logs a cross-validation error using the coupling data and the mapping matrix. This can be useful for tuning the basis function. Note that this might become computationally expensive.");
.setDocumentation("Computes and logs a cross-validation error using coupling data and the mapping matrix. This can be useful for tuning the basis function. Note that this might become computationally expensive.");

To make clearer that the mapping matrix is static and the data dynamic.

Copy link
Member

Choose a reason for hiding this comment

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

Could we give an estimate on the "computationally expensive"? 5x higher than the mapping without cross validation or similar?

mesh::PtrMesh inputMesh,
mesh::PtrMesh outputMesh);

/// Evaluates a conservative mapping and agglomerates the result in the given output data
void mapConservative(const time::Sample &inData, Eigen::VectorXd &outData) const;

/// Evaluates a consistent mapping and agglomerates the result in the given output data
void mapConsistent(const time::Sample &inData, Eigen::VectorXd &outData) const;
std::array<double, 3> mapConsistent(const time::Sample &inData, Eigen::VectorXd &outData) const;
Copy link
Member

Choose a reason for hiding this comment

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

The "3" is the number of components, right?
So, for scalar data the last two components will remain unused?

}
std::vector<double> logErrors;
std::copy_n(maxErrors.begin(), inData.dataDims, std::back_inserter(logErrors));
// Do we want to log every iteration about disabled error metrics?
Copy link
Member

Choose a reason for hiding this comment

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

No, why should we? I would remove this completely.

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.

2 participants