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.

Sign up for GitHub

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

Jump to bottom

Changed the method for SkeletonCellFieldPair for AD to work with operations inside mean and jump #800

Merged
merged 5 commits into from
Jun 17, 2022
Merged

Changed the method for SkeletonCellFieldPair for AD to work with operations inside mean and jump #800

merged 5 commits into from
Jun 17, 2022

Conversation

kishore-nori
Copy link
Collaborator

@kishore-nori kishore-nori commented Jun 17, 2022 •
edited
Loading

  • Overloaded the evaluate! method for SkeletonCellFieldPair
  • removed the change_domain call resulting in wrong behaviour with terms involving operations of CellFields inside mean and jump terms
  • added both SingleField and MultiField AD tests compared with ForwardDiff for such terms

@kishore-nori
Added methods for Skeleton AD with operation inside mean and jump
163a42d 
* This involved removing the change_domain method SkeletonCellFieldPair
* Adding an evaluate! method for SkeletonCellFieldPair
* Adding tests for the feature in FEAutodiff and also MultiField case
@kishore-nori
removed an old comment and commented code
b1a266a
@amartinhuertas
Copy link
Member

Hi @kishore-nori ! Thanks for your work! Can you please edit NEWS.md?

@amartinhuertas amartinhuertas self-requested a review June 17, 2022 03:40
@kishore-nori
Copy link
Collaborator Author

Hi @kishore-nori ! Thanks for your work! Can you please edit NEWS.md?

Done!

@kishore-nori kishore-nori changed the title Changed the method for SkeletonCellFieldPair for AD to work with operations inside mean and jump Changed the method for SkeletonCellFieldPair for AD to work with operations inside mean and jump Jun 17, 2022
amartinhuertas
amartinhuertas requested changes Jun 17, 2022
View reviewed changes
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 your work. We are almost there. I only made a pair of comments that you have to address.

@kishore-nori
Copy link
Collaborator Author

hi @kishore-nori ! Thanks for your work. We are almost there. I only made a pair of comments that you have to address.

Thank you for the review Alberto and the comments. I ll improve the comments and think more about the get_data method for SkeletonCellFieldPair, atleast from the tests on various triangulations, I don't see any side effects!

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2022 •
edited
Loading

Codecov Report

Merging #800 (d125d51) into master (618d57c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #800   +/-   ##
=======================================
  Coverage   88.22%   88.23%           
=======================================
  Files         159      159           
  Lines       17757    17763    +6     
=======================================
+ Hits        15667    15673    +6     
  Misses       2090     2090
Impacted Files Coverage Δ
src/CellData/SkeletonCellFieldPair.jl 79.06% <100.00%> (+3.39%) ⬆️

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

@kishore-nori
Copy link
Collaborator Author

kishore-nori commented Jun 17, 2022 •
edited
Loading

hi @kishore-nori ! Thanks for your work. We are almost there. I only made a pair of comments that you have to address.

Hi Alberto! I improved the comments and explained the choice of overloads and why there are no side effects, please check when you are free.

@amartinhuertas amartinhuertas merged commit 26a1886 into gridap:master Jun 17, 2022
@amartinhuertas amartinhuertas mentioned this pull request Jun 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amartinhuertas amartinhuertas amartinhuertas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kishore-nori @amartinhuertas @codecov-commenter