Skip to content

Fixes to bott.c#29

Merged
stevengj merged 4 commits intoNanoComp:bottfrom
thchr:bott
Sep 19, 2017
Merged

Fixes to bott.c#29
stevengj merged 4 commits intoNanoComp:bottfrom
thchr:bott

Conversation

@thchr
Copy link
Contributor

@thchr thchr commented Aug 29, 2017

  1. Initialize 'bott' number_list to zero before summation-loop.
  2. Un and Wn matrix dimension fix before sqmatrix_resize().

I am unsure why the CHECK in sqmatrix_resize didn't produce an error before.

mpb/bott.c Outdated
sqmatrix_copy(Un, U);
sqmatrix_copy(Wn, W);
Un.p = n_bands; /* sqmatrix_copy does not change the .p field (but _we_ do at every iteration */
Wn.p = n_bands; /* when we resize below), so to correct for that we must do it ourselves here */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand, sqmatrix_resize already sets the .p field. So why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the matrices Un and Wn are copied at lines 98-99 their size still equals the n from the last iteration; then, when we do sqmatrix_resize(..) with preserve_data = 1 the resize/copy loop is inconsistent because it refers to an erroneous size of the original matrix (U or W).

I don't understand why the sqmatrix_copy(..) call doesn't fail though when Un.p and U.p (or, equivalently, Wn and W) must differ after the first iteration (i.e. why does the call to the below not fail at the CHECK?)

/* A = B */
void sqmatrix_copy(sqmatrix A, sqmatrix B)
{
     CHECK(A.p == B.p, "arrays not conformant");
     blasglue_copy(A.p * A.p, B.data, 1, A.data, 1);
}

Copy link
Contributor Author

@thchr thchr Aug 29, 2017

Choose a reason for hiding this comment

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

Or, more briefly: sqmatrix_resize only sets the .p field after it has carried out the preserve_data = 1 part: that part assumes that the .p field reflects the actual size of the original .data field.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(I had forgotten that sqmatrix_copy didn't reset the p value.)

I don't understand why the check isn't failing either, unless checks are disabled (CHECK_DISABLE is defined in your config.h file)....

Can you insert a printf in the sqmatrix_copy call to see what is happening?

In any case, can you change it to put a Un.p = Wn.p = n_bands; line before the sqmatrix_copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CHECK issue seems to have originated with this commit 225be56#diff-c1263b161d86930f3050f951a96fe060 (line 63). I had been comparing to the sqmatrix_copy(..) on the master branch, rather than the bott branch; silly. I don't recall why we made that change then.

Anyway, I've reverted that change and reinstated the original CHECK and moved the n_bands-reassignment of the .p fields up so it's made before we call sqmatrix_copy(..). This seems to provide consistent fix?

mpb/bott.c Outdated
sqmatrix_resize(&S2, n, 0);
sqmatrix_AeBC(S1, Wn, 0, Un, 0);
sqmatrix_resize(&S1, n, 0); /*the .p-field-issue has no effect on scratch mat- */
sqmatrix_resize(&S2, n, 0); /*rices, as their initial value is unimportant */
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't split up "matrices" across two lines in a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

@stevengj stevengj merged commit d1df5a8 into NanoComp:bott Sep 19, 2017
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