-
Notifications
You must be signed in to change notification settings - Fork 40
few minor changes to run example code in FAIRFEM #5
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
base: master
Are you sure you want to change the base?
Conversation
1. ‘yn’ is not defined when wc is empty. Therefore, xn should be updated by yRef. 2. Addpath for add-ons folders during FAIR startup. 3. solver is defined as a char, not string. Probably, this issue has recently arises because of structural changes in R2017a.
These changes are required to use matrix-free functionality of hyper elastic regularization function in the add-on FEMAPP (Only for 2D). To verify the changes, the file “EFEM_SSD_MBvsMF.m” compares the results from MF and MB.
These are minor changes to run matrixfree functionality of FEMPIRE. But, data fidelity term is still not matrixfree. Prob, this involves little more work. Will come back to this later. Added a file to compare results from MF and MB.
|
thanks for this excellent job. Can you please resolve the conflict and clean up the .m~ files? |
| @@ -0,0 +1,38 @@ | |||
| % ================================================================================== | |||
| % (c) Lars Ruthotto 2012/07/26, see FAIR.2 and FAIRcopyright.m. | |||
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.
can you please update the description of the examples and use the new header from the other examples? We decided to let Github keep track of the authorship for us.
Please add a brief description about what this example is supposed to do.
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.
Sure. I will do that
| @@ -0,0 +1,38 @@ | |||
| % ================================================================================== | |||
| % (c) Lars Ruthotto 2012/07/26, see FAIR.2 and FAIRcopyright.m. | |||
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.
See comment below about the description of the example.
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.
Sure. I will do that.
add-ons/FAIRFEM/FEMPIREobjFctn.m
Outdated
| dJ = reshape(Mesh.mfPi(reshape(dD',[],Mesh.dim),'C'),1,[]) + dS; | ||
| %P = @(x) reshape(Mesh.mfPi(reshape(x,[],dim),'C'),[],1); | ||
| %dTcmod = P((sdiag(det)*dT)')' + sdiag(Tc)*dDet; | ||
| P = kron(speye(dim),Mesh.PC); |
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.
this could be a costly operation (which will be done in each call!). In principle, P is only used once and we have matrixFree code for it.
I asssume dT below is sparse matrix of size n by dim*n, where n=prod(m). See if, supplying 'matrixFree',1 in the interpolation would render this a dense n by dim array. Then you can apply P or P' to each column in a matrix free fashion.
This should enable you to avoid the sdiag as well. Can you give this a try?
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.
Certainly. I will work on it. Thanks for suggestion.
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 have modified the code. Please see the new commit.
|
Hey Hari, I resolved the conflicts and added three problems. The one concerning the documentation are critical. See if you also want to address the one regarding the performance. |
Matrixfree implementations of - derivative of determinants, - diagonal of hessian matrix, and - Hessian matrix
|
Hi Lars, I was busy with my work. Sorry for delay. Now, I have modified the matrixfree implementation. Please have a look. Let me know if something else needs to be done. Thanks, |
|
Hi Hari, I have one comment before we can merge this (already did two minor changes to this). My goal is to finish this soon, but it's important to get things right, I think. I noticed that your code for computing the diagonal of the Hessian is only for 2D problems. It would be great if we could extend this to 3D, where matrix-free methods are most attractive. I believe that our mesh classes provide the necessary functionality already. See one change I did to average quantities from the nodes to the barycenters of the elements. Can you help me modify the remainder of this code accordingly and write a small test file for 3D? I have push access to your repo so we can work on this together. Thanks a lot, |
* 'master' of https://github.com/hariagr/FAIR.m: another minor edit two minor changes
Implement matrix free code to evaluate determinant. Added a script “verifyMF_getdeterminant” to verify results
Modified HyperElasticFEM function. Work to do: MatrixFree code for diagonal elements of Hessian matrix.
|
Hi Hari! Thanks for your hard work and patience. It's getting close I think. One final thing that I see is to make the function getDiag in FEMPIREobjFctn work for 3D problems. If you take a look at EFEM_FEMPIRE_Mice3D the code runs until the Hessian solve. I think we need a switch statement here and implement the 2D and 3D separately. Best, |
MatrixFree code for diagonal of Hessian matrix is implemented. We may have an issue in the implementation. - if we set only alphaVolume parameter to check the accuracy of Matrixfree code with MB code, there is a slight error in the calculation. You can run hyperelastic.m to see the error norm. It appears that the differences are high only for few nodes. These nodes varies with different yc/yn values. Generally, d2Svolume has very high values. Small numerical errors in least decimal points are probably showing up. These are few quick observations.
Most of the pieces required for MatrixFree code are completed now. Run EFEM_FEMPIRE_Mice3D.m. I have not done rigorous testing so far. Specially for 3D code, few of the calculations are being done multiple times. May be we can restructure the code little bit to reduce computational time.
|
Thanks, Hari. This works for me! Great job ! |
|
Do you want me to merge this right now, or should I wait for performance improvements? I've also noticed that the matrix free code does not run for non mass-preserving problems since there is no tailored solver. On a different note: Did you compare your code for computing the diagonal of the distance term with getDiagVolume in hyperElasticFEM.m ? Therein, I think we do not recompute things that are not needed. Might help in improving the efficiency for the objective function as well. One idea could be to pull this code out of the regularizer and call it from both functions. What do you think? |
|
Hi Lars,
I can mainly see these changes right now to improve efficiency. I didn't check the code for non mass-preserving problems. I will try to look at it asap. There is no hurry from my side for merging the code. Probably, these changes should not take too much time. We can wait and merge once we are done with our changes. Hari |
|
Hi Hari,
|
Moved getBasisGradient and Cofactor3D functions in a separate script file. Gradient of basis functions are now computed only once in the entire function call, by moving these calculations to MESH structure main file. NOTE: There is a slight difference between estimates coming out from MatrixFree and Non-MatrixFree code, in the 3D case.
|
Hi Lars, I have changed the code. dphi is now stored in the mesh structure. cofactor3D code is separated out. In the last comment I wrote that the Matrixfree code (MF) and non-matrifree (MB) code results are not matching. But, that is not right. They are matching!! By mistake, I used different regularization parameter for MB and MF code during verification. Hari |
Minor changes in FEMobjFctn.m for MatrixFree functionality and added a PCG solver function.
|
Hi Lars, In the MLIRFEM script, image pixel values on a coarse grid are computed at two steps, i.e.,
Instead of this, we can probably define pixels values directly on the FEM triangular mesh and compute approximation on a coarse FEM mesh. Probably, this may results in a more accurate approximation. I am not sure how much beneficial it would be in view of registration accuracy. Probably, you can comment on it. Thanks, |
|
Checked out Hari's branch today. I have some updates to hyperElasticFEM.m to add. Specifically, I will push code that has matrix-based and matrix-free implementations with support for 2D and 3D problems. This includes function calls to extract the diagonal of the Hessian in the matrix case for both 2D and 3D. Will upload as soon as I have everything compatible with the branch. |
|
Perfect. Thank you both for your help!
…On Mon, Jul 16, 2018 at 3:47 PM, James Herring ***@***.***> wrote:
Checked out Hari's branch today. I have some updates to hyperElasticFEM.m
to add. Specifically, I will push code that has matrix-based and
matrix-free implementations with support for 2D and 3D problems. This
includes function calls to extract the diagonal of the Hessian in the
matrix case for both 2D and 3D. Will upload as soon as I have everything
compatible with the branch.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#5 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEDYB75JIQ01wz3_CdeV7odVmnapEBjbks5uHO24gaJpZM4OTrbw>
.
|
‘yn’ is not defined when wc is empty. Therefore, xn should be
updated by yRef.
Addpath for add-ons folders during FAIR startup.
solver is defined as a char, not string. Probably, this issue has
recently arises because of structural changes in R2017a.