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

Homogenize Python Bindings for DirichletValues + Support for fixing Subspace Basis #305

Merged
merged 19 commits into from
Jul 18, 2024
Merged

Homogenize Python Bindings for DirichletValues + Support for fixing Subspace Basis #305

merged 19 commits into from
Jul 18, 2024

Conversation

henrij22
Copy link
Collaborator

@henrij22 henrij22 commented Jun 25, 2024 •
edited
Loading

  • homogenize Python Bidnings into fixBoundaryDOFs function
  • add properties for size and container to DirichletValues in Python
  • add missing interface methods to DirichletValues in Python (fixIthDOF)
  • add support for Subspace Basis fixing to DirichletValues into C++
  • support subspace basis python bindings with DirichletValues
  • expose interface for inhomogenious DBC to Python (different PR)

This also relates to issue #277

@henrij22 henrij22 self-assigned this Jun 25, 2024
@henrij22 henrij22 added feature New feature fix labels Jun 25, 2024
@henrij22 henrij22 linked an issue Jun 25, 2024 that may be closed by this pull request
python bindings for dirichlet values class #301
Closed
@codecov Codecov
Copy link

codecov bot commented Jun 25, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.38%. Comparing base (47c1189) to head (605ae02).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #305   +/-   ##
=======================================
  Coverage   91.37%   91.38%           
=======================================
  Files          64       64           
  Lines        2262     2263    +1     
=======================================
+ Hits         2067     2068    +1     
  Misses        195      195
Flag Coverage Δ
tests 91.38% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@henrij22 henrij22 marked this pull request as ready for review June 26, 2024 09:44
@henrij22 henrij22 requested a review from a team June 26, 2024 09:44
@henrij22 henrij22 mentioned this pull request Jul 1, 2024
Closed
3 tasks
rath3t
rath3t requested changes Jul 5, 2024
View reviewed changes
Copy link
Collaborator

@rath3t rath3t left a comment

Choose a reason for hiding this comment

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

This looks already quite nice thanks! My suggestions are more or less only small cosmetic changes.

ikarus/python/dirichletvalues/dirichletvalues.hh Outdated
using FixBoundaryDOFsWithIntersectionFunction =
std::function<void(Eigen::Ref<Eigen::VectorX<bool>>, int, LocalViewWrapper&, const Intersection&)>;

// Disambiguate by number of arguments, as casting doesn't properly work with functions
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you give a reference, where it is indicated that casting does not work? this is s bold statement that ther is some bug in pybind11 without more explanation, since this should work. maybe crwte a small godbolt ex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So what I tried was to try catch the casts into the different function types. This didn't really work well because the function always got casted into the first possible function without throwing a conversion error, there was then a runtime error later when calling said function.
This issue thread supports this: pybind/pybind11#3035

It looks like py::function is always being casted to first overload in resolution order.

In this case I think disambiguating by number of arguments works quite well, I can of course shorten the comment

ikarus/utils/dirichletvalues.hh Outdated Show resolved Hide resolved
ikarus/utils/dirichletvalues.hh Outdated Show resolved Hide resolved
ikarus/python/dirichletvalues/dirichletvalues.hh Outdated Show resolved Hide resolved
ikarus/utils/dirichletvalues.hh Outdated Show resolved Hide resolved
ikarus/python/dirichletvalues/dirichletvalues.hh Outdated Show resolved Hide resolved
ikarus/python/dirichletvalues/dirichletvalues.hh Outdated Show resolved Hide resolved
ikarus/python/test/testdirichletvalues.py Outdated Show resolved Hide resolved
ikarus/python/dirichletvalues/dirichletvalues.hh Outdated Show resolved Hide resolved
tests/src/testdirichletvalue.cpp Outdated Show resolved Hide resolved
@henrij22
Copy link
Collaborator Author

So this should be more or less ready.
I also added the missing bindings for size to the flatassemblers

rath3t
rath3t requested changes Jul 15, 2024
View reviewed changes
Copy link
Collaborator

@rath3t rath3t left a comment

Choose a reason for hiding this comment

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

Can you have a look at these final comments please

@henrij22 henrij22 force-pushed the feature/dirichletValues branch 4 times, most recently from a814144 to 390f2e1 Compare July 17, 2024 13:26
henrij22 and others added 13 commits July 18, 2024 13:39
@henrij22 henrij22 mentioned this pull request Jul 18, 2024
Open
5 tasks
@henrij22 henrij22 enabled auto-merge (squash) July 18, 2024 12:03
@henrij22 henrij22 disabled auto-merge July 18, 2024 13:07
@henrij22 henrij22 enabled auto-merge (squash) July 18, 2024 15:16
tarun-mitruka
tarun-mitruka reviewed Jul 18, 2024
View reviewed changes
ikarus/python/test/dirichletvaluetest.py
def fixDOFFunction(basis, vec):
vec[0] = True

dirichletValues2.fixDOFs(fixDOFFunction)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice. thanks

@henrij22 henrij22 disabled auto-merge July 18, 2024 16:05
@henrij22 henrij22 enabled auto-merge (squash) July 18, 2024 16:54
@henrij22 henrij22 merged commit 29a1a7c into main Jul 18, 2024
18 of 19 checks passed
@henrij22 henrij22 deleted the feature/dirichletValues branch July 18, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rath3t rath3t rath3t approved these changes

@tarun-mitruka tarun-mitruka tarun-mitruka approved these changes

Assignees

@henrij22 henrij22

Labels
feature New feature fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

python bindings for dirichlet values class
3 participants
@henrij22 @tarun-mitruka @rath3t