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

[MEDApp] Minor Fixes #11580

Merged
merged 8 commits into from
Sep 23, 2023
Merged

[MEDApp] Minor Fixes #11580

merged 8 commits into from
Sep 23, 2023

Conversation

matekelemen
Copy link
Contributor

  • remove the dummy definitions of MPI_Comm and MPI_Info that lead to multiple definitions. Forward declaration also fails because med.h indirectly includes mpi.h that defines these as typedef structs that are incompatible with structs. I haven't tried this on non-MPI builds.
  • update testing macros
  • delete a try-catch-rethrow block from a destructor (destructors are implicitly noexcept)

@philbucher
Copy link
Member

I will review tonight, sorry for the delays

philbucher
philbucher previously approved these changes Sep 21, 2023
Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

We need to figure out a better solution for the MPI-issue, but thx otherwise

Comment on lines 17 to 18
//struct MPI_Comm;
//struct MPI_Info;
Copy link
Member

Choose a reason for hiding this comment

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

the issue here is when you compile without MPI

I am not sure how we can go about this. Maybe ifdef? Might be the only option we have for now, can you give it a try?

Also, how did you install the med-lib? By hand or via the package manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe ifdef?

Yep, I think we'll have to resort to the preprocessor.

Also, how did you install the med-lib? By hand or via the package manager?

I installed salome-medcoupling (should include both the serial and OpenMPI med lib) using the system package manager.

Copy link
Member

Choose a reason for hiding this comment

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

Ok ifdef it is (at least for now, until someone complains :D), can you please add it?

Also, can you please add how you installed the lib to the readme? I did it already for ubuntu

@matekelemen
Copy link
Contributor Author

I found that the created geometries were automatically assigned IDs (really high ones) that eventually lead to exceptions when trying to output the resulting ModelPart to MDPA via ModelPartIO::WriteModelPart, so I added explicit IDs for the created geometries.

The current impl obviously won't work with ModelParts that are already populated, but we can worry about those later because the Nodes are created in the same manner.

@philbucher
Copy link
Member

I found that the created geometries were automatically assigned IDs (really high ones) that eventually lead to exceptions when trying to output the resulting ModelPart to MDPA via ModelPartIO::WriteModelPart, so I added explicit IDs for the created geometries.

The current impl obviously won't work with ModelParts that are already populated, but we can worry about those later because the Nodes are created in the same manner.

ok thanks that makes sense
Lets add a check in read mode if nodes already exist (I thought I had added that one already). Can you please take care of this one?

Ids of entities and mypy, my programming arch-enemies :D

@philbucher philbucher mentioned this pull request Sep 21, 2023
Base automatically changed from med-application to master September 21, 2023 21:13
@philbucher philbucher dismissed their stale review September 21, 2023 21:13

The base branch was changed.

@matekelemen matekelemen merged commit 54f69c8 into master Sep 23, 2023
14 of 16 checks passed
@matekelemen matekelemen deleted the med/fixes branch September 23, 2023 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants