-
Notifications
You must be signed in to change notification settings - Fork 100
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
Extended the jacobian
to functionals involving Skeleton terms
#803
Conversation
* Added corresponding tests compared with ForwardDiff * The high-level api remains the same
…Gridap.jl into AD-for-SkeletonIntegration
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.
Hi @kishore-nori ! thanks for the PR! Only minor comments.
Edit the NEWS.md file, please |
Codecov Report
@@ 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
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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. |
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. |
Hi Alberto, thank you for the logs. Yeah I realised that the testing times have increased due to the new |
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
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.
CI tests now take 45 mins. Well done @kishore-nori !
That's great! I think the wrapping of |
jacobian
for functionals involving inetgration over Skeleton terms can be handled now