-
Notifications
You must be signed in to change notification settings - Fork 92
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
Simplify evaluate_at_grid_nodes #1097
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1097 +/- ##
==========================================
- Coverage 93.57% 93.57% -0.01%
==========================================
Files 39 39
Lines 6074 6071 -3
==========================================
- Hits 5684 5681 -3
Misses 390 390 ☔ View full report in Codecov by Sentry. |
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.
Looks like good changes to me!
GitHub/CodeCov complains that it isn't covered by tests, that looks odd to me?
Edit: NVM, the codecov report show these lines as covered
f831eed
to
0972061
Compare
Co-authored-by: Knut Andreas Meyer <[email protected]>
0972061
to
cd9812f
Compare
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.
LGTM if formatting is fixed such that runic / pre-commit workflows pass.
I guess tests should test the vtk path, but did you also check manually that export of the solution gives correct vtk output?
Yes, the vtk output looks correct (checked manually). And we have tests that check if the hash of vtk-files changes, right? |
True, I wasn't sure if the writing of solution was covered by those, but seems so! |
There were some hacks in
evaluate_at_grid_nodes
for embedded element which I don't think is needed anymore.Also added
shape_value_type(::Interpolation, T)
which I think will be required for #958 also.