-
Notifications
You must be signed in to change notification settings - Fork 279
[Hdf5] Fix windows compilation #14091
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
base: master
Are you sure you want to change the base?
Conversation
|
I'm compiling this branch, will let you know |
|
I am getting an Here's the log: |
|
Ok, compilation errors should be solved, but not sure about the CI testes failing, I suspect there is some mismatch with the overload of the cheks in the function |
|
I am still getting some Here's the log: |
sunethwarna
left a comment
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.
Thanks @roigcarlo , and sorry for being late....
| namespace Internals | ||
| { | ||
| std::string AddMissingAndGetPrefix(Parameters Settings) | ||
| std::string AddMissingAndGetPrefix(Parameters & Settings) |
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.
Is this necessary?
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.
Yep, otherwise the copy contructor implicitly calls Get<>
| KRATOS_TRY | ||
|
|
||
| const std::string prefix = Settings["prefix"].Get<std::string>(); | ||
| const std::string prefix = Settings["prefix"].GetString(); |
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.
Curious: Was the previous causing problems?
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.
Getstd::string is not exposed, hence cannot be called.
|
Sadly I cannot find the errors, so I reverted the changes to hdf5 so it keeps using unsigned char and added it to the data communicator interface. @Rbravo555 This should make the tests ok again, if it works I will extend the API interface for win to accommodate the lasts errors you sent. |
…y which caused write to fail
|
@sunethwarna I've added a set of changes to the app: This can be the cause of other tests randomly failing with extremely low fail rates that we have been seeing if they follow a similar mechanism while creating the modelparts. We should keep an eye on them. Lastly, there seems to be still a deadlock while running in mpi under some scenarios. For now it eludes me why, if you could take a look could be super helpful 🙏 |
I have attemped the compile just now with the latest changes with the following error log: |
And probably fixing HDF5 🤞
| // Complete interface for basic types | ||
|
|
||
| KRATOS_BASE_DATA_COMMUNICATOR_DECLARE_PUBLIC_INTERFACE_FOR_TYPE(char) | ||
| KRATOS_BASE_DATA_COMMUNICATOR_DECLARE_PUBLIC_INTERFACE_FOR_TYPE(unsigned char) |
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.
Please @roigcarlo remove this because the DataCommunicator interface does not support unsigned char reductions as it should be used with MPI_BYTE or MPI_UNSIGNED_CHAR
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.
Ok will do once test pass
| if (availability_global_counts_pair[0] == availability_global_counts_pair[2]) { | ||
| if (non_available_count == 0) { |
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.
@sunethwarna I am pretty sure this was the problems with the testing, because it was accessing [2] of the availability vector. Also, if the interface failed to do the casting from unsigned char to a type supported by the DataCommmunicator it did not calculate well the global values and failed while assigning container_data_availability (it also was checking a [2]).
This should fix all. I opted to change to because apparently the DataCommunicator interface fails with this type specifically (Probably this is why it wasn't there in the first place).
📝 Description
@Rbravo555 this Fixes #14089 (or should, can't test exaustively). If you can try it and give feedback I'd appreaciate.
@sunethwarna There is big change in hdf5_points_data. I've merged the source and header in one file. I am afraid that there is no other solution for this class in particular, because
VertexContainerCoordinateIOis derived fromPointsData<VertexIO>which causes the later to be partialy defined until the compiler sees the definition ofPointsData<VertexIO>, which triggers the error and the explicit instantiation cannot be moved before the definition ofVertexContainerbecause its base class needs to come before...Rest of the problems were most warnings because stf::vector especializations which causes functions not being able to resolve overloads correctly.