Use combined W matrix for primary and secondary data in IQN #2010

Fujikawas · 2024-04-21T20:45:02Z

Main changes of this PR

The secondary data is handled together with primary data in the W matrix both in IQN-ILS and IQN-IMVJ method.

Motivation and additional information

Relating to #1996

To do

Check if the secondaryDataIDs and _values with only primary data are still needed; -> Yes, for computing V, warn zero update of V;
The preconditioning of Wtil should still be done, currently commented out;
With the tests with secondary data in Add secondary data to exsting QN tests #2006, this method could work. Still the QNTest9EmptyPartition which is to test SVD-restart failed. Other tests with SVD could work.

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 test to cover the proposed changes in our test suite.
I squashed / am about to squash all commits that should be seen as one.

Reviewers' checklist

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

uekerman

Looks promising!
I guess the SVD changes could go to a separate refactoring PR.

src/acceleration/BaseQNAcceleration.hpp

uekerman

Nice to see how these changes reduce code duplication 👍

Still a bit of work to do, especially for parallel runs I guess, see below.

Requires a changelog as we now have support of secondary data for IMVJ

uekerman · 2024-04-25T13:56:38Z

src/acceleration/BaseQNAcceleration.hpp

  virtual std::vector<int> getDataIDs() const
  {
-    return _dataIDs;
+    return _primaryDataIDs;
  }


Let's change the name of this function as well.

For the whole acceleration method? Other under-relaxation methods also use this function

For Aitken, we in principle also have the primary vs secondary story.

src/acceleration/BaseQNAcceleration.cpp

uekerman · 2024-04-25T14:13:32Z

src/acceleration/BaseQNAcceleration.cpp

-    size_t unknowns = _dimOffsets[utils::IntraComm::getRank() + 1] - _dimOffsets[utils::IntraComm::getRank()];
-    PRECICE_ASSERT(entries == unknowns, entries, unknowns);
+    // size_t unknowns = _dimOffsets[utils::IntraComm::getRank() + 1] - _dimOffsets[utils::IntraComm::getRank()];
+    // PRECICE_ASSERT(entries == unknowns, entries, unknowns); //?


This is a tricky point. Should _dimOffsets include secondary data or not? Probably makes a difference for IMVJ. Let's try to also add a test with secondary data that runs in parallel.

Since it's used to initialize the size of Wtil, I would say that the _dimOffsets should include the secondary data. But I'm a little confused with this function now:

precice/src/acceleration/BaseQNAcceleration.cpp

Lines 626 to 632 in 8458db5

int BaseQNAcceleration::getLSSystemRows()

{

if (utils::IntraComm::isParallel()) {

return _dimOffsets.back();

}

return _primaryResiduals.size();

}

In the extended tests, the parallel tests with IMVJ include also secondary data. They could work with this implementation here(secondary data included in the _dimOffsets). This assert should be modified a bit then.

uekerman · 2024-04-25T14:15:15Z

src/acceleration/BaseQNAcceleration.cpp

+  PRECICE_ASSERT(_oldPrimaryResiduals.size() == _oldPrimaryXTilde.size(), _oldPrimaryResiduals.size(), _oldPrimaryXTilde.size());
+  PRECICE_ASSERT(_primaryValues.size() == _oldPrimaryXTilde.size(), _primaryValues.size(), _oldPrimaryXTilde.size());
+  PRECICE_ASSERT(_oldPrimaryValues.size() == _oldPrimaryXTilde.size(), _oldPrimaryValues.size(), _oldPrimaryXTilde.size());
+  PRECICE_ASSERT(_primaryResiduals.size() == _oldPrimaryXTilde.size(), _primaryResiduals.size(), _oldPrimaryXTilde.size());

  // assume data structures associated with the LS system can be updated easily.

  // scale data values (and secondary data values)


Is this comment correct? I don't think that there is any scaling in there.

Only concatenate happens until the next comment

uekerman · 2024-04-25T14:16:56Z

src/acceleration/BaseQNAcceleration.cpp

    _residuals *= _initialRelaxation;
-    _residuals += _oldValues;
-    _values = _residuals;
+    _values = _oldValues + _residuals;


Is this correct? Now _residuals has a different value.

From the test results I didn't see difference. If you mean that the _residuals is not updated here, I see that the _residuals would be calculated from _values and _oldValues by updating differences and is not stored like _values, so the step here still works.

If you mean that the _residuals is not updated here

Exactly.

If such a change is unrelated to this PR, I would always do it separately as a refactoring. Otherwise, we might get side effects.

From the test results I didn't see difference

Remember that tests are never perfect.

uekerman · 2024-04-25T14:22:14Z

src/acceleration/impl/ParallelMatrixOperations.hpp

@@ -136,7 +136,6 @@ class ParallelMatrixOperations {

    PRECICE_ASSERT(_needCyclicComm);
    PRECICE_ASSERT(leftMatrix.cols() == q, leftMatrix.cols(), q);
-    PRECICE_ASSERT(leftMatrix.rows() == rightMatrix.cols(), leftMatrix.rows(), rightMatrix.cols());


Removing this doesn't feel good.

In the comment it says that For multiplication W_til * Z = J, p = r = n_global, q = m, which is not the case now. I'm not so sure why this assertion is necessary for the operation unless these two variables are mixed used. Need to check.

uekerman · 2024-04-25T14:23:56Z

src/acceleration/impl/Preconditioner.hpp

+      PRECICE_WARN_IF(M.cols() < (int) _weights.size(), "Matrix and weights size mismatch, only part of the preconditioning weights will be used.");
+      PRECICE_WARN_IF(M.cols() > (int) _weights.size(), "Matrix and weights size mismatch, only part of the matrix will be preconditioned.");


Let's find a better solution here. Such warnings will confuse 99% of users.

uekerman · 2024-04-25T14:24:55Z

src/acceleration/impl/Preconditioner.hpp

Could we add unit tests to cover these changes?

uekerman · 2024-04-25T14:26:01Z

src/acceleration/impl/SVDFactorization.hpp

I still think it would be better to do these changes in a separate PR before this one. And add unit tests for the new functionality.

How about do one PR for SVD and one preconditioning and one for IQN itself

use combined W for primary and secondary data in IQN

278d0d8

github-actions bot assigned Fujikawas Apr 21, 2024

uekerman reviewed Apr 22, 2024

View reviewed changes

src/acceleration/BaseQNAcceleration.hpp Show resolved Hide resolved

src/acceleration/BaseQNAcceleration.hpp Outdated Show resolved Hide resolved

Jun Chen added 2 commits April 25, 2024 10:11

rename variables and allow unmatched size in preconditioner

2b571d2

rename variables and remove secondaryDataIDs

8458db5

Fujikawas requested a review from uekerman April 25, 2024 09:24

uekerman requested changes Apr 25, 2024

View reviewed changes

Fujikawas mentioned this pull request Apr 29, 2024

Extend SVD to cover input multiplicators with different sizes #2014

Merged

8 tasks

Merge branch 'develop' into IQN-secondary-data

db6eb8d

Fujikawas mentioned this pull request May 8, 2024

Relax assertions in preconditioner for ummatched weight and target sizes #2020

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use combined W matrix for primary and secondary data in IQN #2010

Use combined W matrix for primary and secondary data in IQN #2010

Fujikawas commented Apr 21, 2024 •

edited

uekerman left a comment

uekerman left a comment

uekerman Apr 25, 2024

Fujikawas Apr 26, 2024

uekerman May 3, 2024

uekerman Apr 25, 2024

Fujikawas Apr 28, 2024

uekerman Apr 25, 2024

Fujikawas Apr 26, 2024

uekerman Apr 25, 2024

Fujikawas Apr 26, 2024

uekerman May 3, 2024

uekerman Apr 25, 2024

Fujikawas Apr 26, 2024

uekerman Apr 25, 2024

uekerman Apr 25, 2024

uekerman Apr 25, 2024

Fujikawas Apr 26, 2024 •

edited

	int BaseQNAcceleration::getLSSystemRows()
	{
	if (utils::IntraComm::isParallel()) {
	return _dimOffsets.back();
	}
	return _primaryResiduals.size();
	}

		PRECICE_WARN_IF(M.cols() < (int) _weights.size(), "Matrix and weights size mismatch, only part of the preconditioning weights will be used.");
		PRECICE_WARN_IF(M.cols() > (int) _weights.size(), "Matrix and weights size mismatch, only part of the matrix will be preconditioned.");

Use combined W matrix for primary and secondary data in IQN #2010

Are you sure you want to change the base?

Use combined W matrix for primary and secondary data in IQN #2010

Conversation

Fujikawas commented Apr 21, 2024 • edited

Main changes of this PR

Motivation and additional information

To do

Author's checklist

Reviewers' checklist

uekerman left a comment

Choose a reason for hiding this comment

uekerman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fujikawas Apr 26, 2024 • edited

Choose a reason for hiding this comment

Fujikawas commented Apr 21, 2024 •

edited

Fujikawas Apr 26, 2024 •

edited