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

Add some complex compatibility changes #81

Merged
merged 11 commits into from May 16, 2018
Merged

Add some complex compatibility changes #81

merged 11 commits into from May 16, 2018

Conversation

IgorBaratta
Copy link
Member

@IgorBaratta IgorBaratta commented May 15, 2018

  • Add MPI definition for complex double (MPI_DOUBLE_COMPLEX)
  • Allow complex dot products in la::VectorSpaceBasis

Copy link
Member

@garth-wells garth-wells left a comment

Choose a reason for hiding this comment

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

I'm wondering what degree we want to wrap PETSc calls vs just using PETSc. We used to support multiple backends, but now that we're PETSc only maybe we should just use PETSc and petsc4py?


if (size(0) != x.size())
{
log::dolfin_error(
Copy link
Member

Choose a reason for hiding this comment

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

We're not using dolfin_error anymore. Switch to throw std::foo exceptions.

@@ -452,6 +452,35 @@ void PETScMatrix::transpmult(const PETScVector& x, PETScVector& y) const
petsc_error(ierr, __FILE__, "MatMultTranspose");
}
//-----------------------------------------------------------------------------
void PETScMatrix::conjtranspmult(const PETScVector& x, PETScVector& y) const
Copy link
Member

Choose a reason for hiding this comment

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

Could be make the function name closer to the name of the PETSc function?

@@ -29,7 +29,7 @@ void VectorSpaceBasis::orthonormalize(double tol)
// orthonormalized vectors
for (std::size_t j = 0; j < i; ++j)
{
const double dot_ij = _basis[i]->dot(*_basis[j]);
const auto dot_ij = _basis[i]->dot(*_basis[j]);
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to avoid auto to make the type clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess here it should be PETScScalar

@IgorBaratta
Copy link
Member Author

IgorBaratta commented May 15, 2018

I'm wondering what degree we want to wrap PETSc calls vs just using PETSc. We used to support multiple backends, but now that we're PETSc only maybe we should just use PETSc and petsc4py?

I added the function to have a consistency between the complex and real interfaces. But, as PETSc and petsc4py are now both required dependencies, it may be a question of clean up rather than adding new functions for the complex case.
I can readily remove specific functions for complex la, if that is the case.

@garth-wells
Copy link
Member

Maybe we should remove PETScMatrix::transpmult - I don't think we want to wrap the whole PETSc interface. @chrisrichardson @blechta ?

@chrisrichardson
Copy link
Contributor

My question would be: what is it for? Do we need it internally? If not, then it is part of the user interface, which can be done with PETSc/petsc4py directly.

@garth-wells
Copy link
Member

Run clang-format on the code (the config file for clang-format is in the root of the repo).

It's possible to configure editors to apply it for you.

Copy link
Member

@garth-wells garth-wells left a comment

Choose a reason for hiding this comment

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

Nice work.

@IgorBaratta IgorBaratta merged commit 5e98b7b into master May 16, 2018
@IgorBaratta IgorBaratta deleted the igor/complex-la branch May 16, 2018 20:09
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.

None yet

3 participants