Skip to content
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

Extended the jacobian to functionals involving Skeleton terms #803

Merged
merged 4 commits into from
Jun 22, 2022
Merged

Extended the jacobian to functionals involving Skeleton terms #803

merged 4 commits into from
Jun 22, 2022

Conversation

kishore-nori
Copy link
Collaborator

  • jacobian for functionals involving inetgration over Skeleton terms can be handled now
  • Added corresponding tests compared with ForwardDiff
  • The high-level API remains the same

* Added corresponding tests compared with ForwardDiff
* The high-level api remains the same
@amartinhuertas amartinhuertas self-requested a review June 21, 2022 09:44
Copy link
Member

@amartinhuertas amartinhuertas left a comment

Choose a reason for hiding this comment

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

Hi @kishore-nori ! thanks for the PR! Only minor comments.

@amartinhuertas
Copy link
Member

Edit the NEWS.md file, please

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2022

Codecov Report

Merging #803 (8b7d3ba) into master (26a1886) will increase coverage by 0.21%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master     #803      +/-   ##
==========================================
+ Coverage   88.23%   88.45%   +0.21%     
==========================================
  Files         159      159              
  Lines       17763    17865     +102     
==========================================
+ Hits        15673    15802     +129     
+ Misses       2090     2063      -27     
Impacted Files Coverage Δ
src/FESpaces/FEAutodiff.jl 92.72% <95.65%> (+3.71%) ⬆️
src/Polynomials/QGradMonomialBases.jl 99.25% <0.00%> (+0.16%) ⬆️
src/Visualization/Vtk.jl 87.93% <0.00%> (+1.00%) ⬆️
src/CellData/CellFields.jl 91.20% <0.00%> (+1.62%) ⬆️
src/Fields/ApplyOptimizations.jl 57.05% <0.00%> (+2.35%) ⬆️
src/ReferenceFEs/NedelecRefFEs.jl 91.17% <0.00%> (+2.37%) ⬆️
src/CellData/SkeletonCellFieldPair.jl 88.37% <0.00%> (+9.30%) ⬆️
src/Fields/AffineMaps.jl 70.17% <0.00%> (+14.03%) ⬆️

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

@kishore-nori
Copy link
Collaborator Author

Hi Alberto, thank you very much for the help, inputs and discussion for the PR, and for reviewing. I fixed the suggested changes and added further details and pushed them.

@amartinhuertas
Copy link
Member

Hi @kishore-nori ... I am afraid that we are increasing too much testing times in Github Actions with this PR.

Looking at the logs, we have increased total time from 46 min to 1h 5min approx (by 20 mins approx).

See attached picture below to identify where the hot spot is.

Can we try to do a better job here and curate the current set of tests such that we significantly reduce this testing times while still testing the same amount of code base within Gridap kernel? Perhaps we can take a look at code coverage reports (https://app.codecov.io/gh/gridap/Gridap.jl/branch/AD-for-SkeletonIntegration/) to understand which parts of the codes we are going through with each of the tests.

Screenshot from 2022-06-22 11-43-55

@kishore-nori
Copy link
Collaborator Author

kishore-nori commented Jun 22, 2022

Hi Alberto, thank you for the logs. Yeah I realised that the testing times have increased due to the new jacobian for Skeleton functions in FEAutodiffTests.jl. The thing is I put in 5 functionals which reflect most of the complications which are possible in such cases. These cover usual mean and jump terms of uh and vh, terms with combinations of ∇(uh), ∇(vh), Δ(u) and Δ(vh). Additionally, there are test where jump and mean have operations of CellFields inside. One way that I can reduce the run-time is by having 2x2 grid or even 2x1 grid instead of current 3x3 grid. I can change this based on your suggestions. I kept reffe order at 2 to test even laplacians.

@amartinhuertas
Copy link
Member

amartinhuertas commented Jun 22, 2022

One way that I can reduce the run-time is by having 2x2 grid or even 2x1 grid instead of current 3x3 grid. I can change this based on your suggestions.

I am not sure this is going to help much. Note in the logs that 99% of the time is compilation time. So I would say it is more about the amount of specialized code for which you are triggering compilation from your tests, not the size of the test cases.

* removed higher order derivative tests for jacobian with skeleton functionals
* reduced the dimension of problem to 2x2 and order = 1 for skeleton tests
* wrapped repeated test code into a function to reduce compile time
Copy link
Member

@amartinhuertas amartinhuertas left a comment

Choose a reason for hiding this comment

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

CI tests now take 45 mins. Well done @kishore-nori !

@kishore-nori
Copy link
Collaborator Author

That's great! I think the wrapping of assemble(jacobian(...)) into a function might have also helped.

@amartinhuertas amartinhuertas merged commit 02d96ad into gridap:master Jun 22, 2022
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.

3 participants