Towards AD for Skeleton integration terms - gradient added

[kishore-nori]

• Added functionality for performing gradient of functionals involving integrals over Skeleton faces (aimed at resolving issue Gradient of Integration domain contribution involving SkeletonTriangulation #787)
• corresponding tests for the new constructs and functionalities have also been added
• Jacobian and Hessian of functionals involving Skeleton integration terms is work-in-progress
• gradients of functionals involving Skeleton integration of MultiFields is to be done (not yet started)

[amartinhuertas]

Hi @kishore-nori ! Thanks for the PR!

The tests did not pass! https://github.com/gridap/Gridap.jl/runs/6827393445?check_suite_focus=true#step:6:211

Seems an easy fix.

[amartinhuertas]

@kishore-nori nice work! I have completed my first round of review. Please, take into account my minor comments and resubmit.

[kishore-nori]

Currently, there is a problem of requiring circular dependency due to BlockMap usage in src/Arrays/Autodiff.jl. The thing is, I think we ll have to move the autodiff_array_gradient (for Skeleton integration) to FESpaces, may be to FEAutodiff.jl or a different file as we are using BlockMap that is not imported (due ciruclar dependency) into Gridap.Arrays. BlockMap belongs to Gridap.Fields which FESpaces is also using/importing.

The only changes to be done is to import DualizeMap and AutoDiffMap to Gridap.FESpaces. Based on your suggestions for the best way to go ahead, I ll make changes accordingly. This comes with an advantage of being able to SkeletonPair again!

[amartinhuertas]

The only changes to be done is to import DualizeMap and AutoDiffMap to Gridap.FESpaces. Based on your suggestions for the best way to go ahead, I ll make changes accordingly. This comes with an advantage of being able to SkeletonPair again!

We can proceed as you suggest.

[kishore-nori]

Sure Alberto, I ll move it FESpaces. Should I put it in FEAutodiff.jl or in a new file?

[amartinhuertas]

Sure Alberto, I ll move it FESpaces. Should I put it in FEAutodiff.jl or in a new file?

you can put in FEAutodiff.jl along with a comment on why you had to put it there and not in Arrays.

[kishore-nori]

Sure, I ll move it to src/FESpaces/FEAutodiff.jl and add details as suggest on why it has been placed there.

[codecov-commenter]

Codecov Report

Merging #797 (2e52ba8) into master (3916f3c) will decrease coverage by 0.06%.
The diff coverage is 71.21%.

@@            Coverage Diff             @@
##           master     #797      +/-   ##
==========================================
- Coverage   88.24%   88.18%   -0.07%     
==========================================
  Files         158      159       +1     
  Lines       17651    17717      +66     
==========================================
+ Hits        15577    15624      +47     
- Misses       2074     2093      +19     

Impacted Files	Coverage Δ
src/Arrays/Autodiff.jl	100.00% <ø> (ø)
src/FESpaces/FESpaces.jl	0.00% <ø> (ø)
src/CellData/SkeletonCellFieldPair.jl	60.52% <60.52%> (ø)
src/FESpaces/FEAutodiff.jl	89.01% <85.71%> (-1.47%)	⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[amartinhuertas]

Sorry this the latest commit. The thing is I need both the outer and inner constructors for convenience, the outer one for normal construction and inner one when I have derivatives of SkeletonCellFieldPair. The derivatives of CellFieldAt return a CellFieldAt so having the inner one would be useful.

ok.

[amartinhuertas]

hi @kishore-nori ... FYI, it seems that Gridap tests got stuck in Github actions. I cancelled them after 2hours and 45 mins.

https://github.com/gridap/Gridap.jl/runs/6856453767?check_suite_focus=true#step:6:210

I have re-run them to see if the problem persists.

[kishore-nori]

Thank you @amartinhuertas, I was curious on why it is taking so much time. Hopefully it is not a bug in the code.

[amartinhuertas]

Can you run FESpaceTests.jl on your local machine?

I am not sure what is going on, but it might be (hypothesis) that you have some sort of infinite recursion among the inner/outer constructions or something like that.

[amartinhuertas]

Also which version of Julia do you have locally? The remote end uses julia version 1.7.3

[kishore-nori]

Yes Alberto, I will test in on my laptop locally in sometime. I am currently having Julia 1.7.2 running.

[kishore-nori]

Hi Alberto, you were right, there was recursive loop between inner and outer constructor of SkeletonCellFieldPair. I tested all the tests that have been affected by changes and they all run fine now, so pushed the changes and hopefully the CI will run fine now.