-
Notifications
You must be signed in to change notification settings - Fork 61
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
Turn on operator partial assembly by default #168
Conversation
09672fe
to
43804da
Compare
fba6b59
to
6a5574b
Compare
deabef5
to
5fc48fe
Compare
6a5574b
to
e321258
Compare
b04a669
to
8ec1ad4
Compare
8a72dc9
to
b2aa993
Compare
b2aa993
to
aacc041
Compare
f2674b5
to
e86da12
Compare
palace/drivers/eigensolver.cpp
Outdated
if (i < iodata.solver.eigenmode.n) | ||
{ | ||
// Only consider the desired number of modes for the error indicator. | ||
estimator.AddErrorIndicator(E, indicator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I realized this is incorrect, since below the indicator is postprocessed for i == 0
which means the value written isn't actually considering all modes. I should move it back outside the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 95dbe4d
… partial assembly for all operators
…ctors for consistency of results across processor counts (where true dof definitions, and thus l2 norm, might change) Also re-baseline cavity results for the new change.
95dbe4d
to
43b7b42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tiny comment fix but otherwise LGTM.
@@ -682,25 +713,20 @@ int ArpackPEPSolver::Solve() | |||
if (!eig) | |||
{ | |||
eig = std::make_unique<std::complex<double>[]>(nev + 1); | |||
perm = std::make_unique<int[]>(nev + 1); | |||
res = std::make_unique<double[]>(nev + 1); | |||
perm = std::make_unique<int[]>(nev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change from nev+1
to nev
, was it previously oversized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, only the eigenvectors need to be eig + 1
for ARPACK. Everything else is sized for nev
and used by on the Palace side.
1.000000000e+00, +9.139656208e-02, +9.139657101e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657101e-02, +1.000000000e+00 | ||
2.000000000e+00, +9.139656208e-02, +9.139657093e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657093e-02, +1.000000000e+00 | ||
3.000000000e+00, +9.139656208e-02, +9.139657091e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657091e-02, +1.000000000e+00 | ||
4.000000000e+00, +9.139656208e-02, +9.139657118e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657118e-02, +1.000000000e+00 | ||
5.000000000e+00, +9.139656208e-02, +9.139657079e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657079e-02, +1.000000000e+00 | ||
6.000000000e+00, +9.139656208e-02, +9.139657070e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657070e-02, +1.000000000e+00 | ||
7.000000000e+00, +9.139656208e-02, +9.139657058e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657058e-02, +1.000000000e+00 | ||
8.000000000e+00, +9.139656208e-02, +9.139657024e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657024e-02, +1.000000000e+00 | ||
9.000000000e+00, +9.139656208e-02, +9.139657046e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657046e-02, +1.000000000e+00 | ||
1.000000000e+01, +9.139656208e-02, +9.139657036e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657036e-02, +1.000000000e+00 | ||
1.100000000e+01, +9.139656208e-02, +9.139657066e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657066e-02, +1.000000000e+00 | ||
1.200000000e+01, +9.139656208e-02, +9.139657004e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657004e-02, +1.000000000e+00 | ||
1.300000000e+01, +9.139656208e-02, +9.139657079e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657079e-02, +1.000000000e+00 | ||
1.400000000e+01, +9.139656208e-02, +9.139657060e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657060e-02, +1.000000000e+00 | ||
1.500000000e+01, +9.139656208e-02, +9.139657051e-02, +0.000000000e+00, +0.000000000e+00, +9.139656208e-02, +1.000000000e+00, +9.139657051e-02, +1.000000000e+00 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every number in here being functionally equal now makes this look a little weird, an argument could be made it's not necessary to write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue keeping the format consistent with the other simulation types is worth it. Also it's a good check to make sure something did not break! Note also, if you're referring to electric vs. magnetic field energy, they are not equal as a result of the error tolerance associated with the eigenvalue solver. Also for problems with lumped elements, they won't be equal in general.
@@ -1,2 +1,2 @@ | |||
Norm, Minimum, Maximum, Mean | |||
+1.831462799e-02, +4.326597058e-04, +1.397659264e-03, +7.450386480e-04 | |||
+1.786937592e-03, +1.175148951e-04, +3.263378876e-04, +2.040337637e-04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good old spectral convergence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked it too!
@@ -1,2 +1,2 @@ | |||
Norm, Minimum, Maximum, Mean | |||
+7.056448953e-01, +1.149467165e-06, +1.689680136e-02, +8.751155027e-04 | |||
+6.039988870e-01, +2.399647627e-06, +2.597476399e-02, +2.443466119e-03 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice demonstration of irregularity, the min max and mean rise, but the norm decreases.
NOTE: Based on #167
All examples use p-refinement in place of mesh refinement now as well.
In order to ensure consistency across runs for eigenvector normalizations, we now normalize with respect to the mass matrix (unit electric field energy). The previous l2-norm on the true dofs is not consistent for high-order Nedelec spaces where the orientations (and thus definitions of the dofs) change based on the partitioning.
Reference issue: #3