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

[StatApp] Getting ready statistics application for CI integration #7094

Merged
merged 37 commits into from
Jun 19, 2020
Merged

[StatApp] Getting ready statistics application for CI integration #7094

merged 37 commits into from
Jun 19, 2020

Conversation

sunethwarna
Copy link
Member

@sunethwarna sunethwarna commented Jun 17, 2020 •
edited
Loading

Hi,

This PR makes StatApp ready for CI integration (linux and windows both).

It has 5 cpp tests, 54 MPI tests takes 1.7 seconds, and 66 OpenMP tests takes 2.22 seconds in my PC.

@sunethwarna sunethwarna requested a review from philbucher June 17, 2020 09:07
@sunethwarna sunethwarna requested a review from a team as a code owner June 17, 2020 09:07
@sunethwarna sunethwarna self-assigned this Jun 17, 2020
@philbucher philbucher added Continuous Integration related to Travis, Appveyor, ... Applications labels Jun 17, 2020
@philbucher
Copy link
Member

if this is to be merged then please also add it to the nightly build

@philbucher
Copy link
Member

as asked here, who are the official maintainers?

@sunethwarna
Copy link
Member Author

I will be the official maintainer for now :). Where should I put it ? In the issue?

@philbucher
Copy link
Member

Having at least two maintainers providing continuous support in case of CI failure.

you need a second maintainer
maybe @mpentek ?

@philbucher
Copy link
Member

meanwhile there are many compiler errors in Win

@sunethwarna
Copy link
Member Author

on it....

@mpentek
Copy link
Member

mpentek commented Jun 17, 2020

I am ok as second maintainer.

@sunethwarna
Copy link
Member Author

@roigcarlo No matter what I do, I cannot get StatisticsApplication to be compiled with Windows. Could you please have a look? I have no experience with Kratos windows compilation. :/

@sunethwarna sunethwarna requested a review from roigcarlo June 18, 2020 14:35
@roigcarlo
Copy link
Member

Sure, let me take a look

@sunethwarna
Copy link
Member Author

@roigcarlo thanks a lot. :) @philbucher if everything is ok to add for CI, could you please approve?

nightly builds are failing not because of stat app.

@philbucher
Copy link
Member

@philbucher if everything is ok to add for CI, could you please approve?

this is not my decision only ;)

But I will take a look at your changes

philbucher
philbucher reviewed Jun 19, 2020
View reviewed changes
applications/StatisticsApplication/custom_methods/spatial_methods.h
rModelPart.GetCommunicator().GetDataCommunicator().SumAll(
r_container.size());
static_cast<double>(r_container.size()));
Copy link
Member

Choose a reason for hiding this comment

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

cannot you use unsigned int here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

i casted it directly to double since the next line I anyways have to deal with the double value, so i can avoid double casting.
return sum * (1.0 / std::max(number_of_items, 1.0));.

applications/StatisticsApplication/custom_methods/spatial_methods.h
rModelPart.GetCommunicator().GetDataCommunicator().SumAll(
r_container.size());
static_cast<double>(r_container.size()));
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

same as above

applications/StatisticsApplication/custom_methods/spatial_methods.h
rModelPart.GetCommunicator().GetDataCommunicator().SumAll(
r_container.size());
static_cast<double>(r_container.size()));
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

same as above

applications/StatisticsApplication/custom_methods/spatial_methods.h
rModelPart.GetCommunicator().GetDataCommunicator().SumAll(
r_container.size());
static_cast<double>(r_container.size()));
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

same as above

applications/StatisticsApplication/custom_methods/spatial_methods.h
rModelPart.GetCommunicator().GetDataCommunicator().SumAll(
r_container.size());
return std::sqrt(global_sum / std::max(static_cast<double>(number_of_items), 1.0));
static_cast<double>(r_container.size()));
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

same as above

applications/StatisticsApplication/custom_methods/spatial_methods.h
rModelPart.GetCommunicator().GetDataCommunicator().SumAll(
r_container.size());
static_cast<double>(r_container.size()));
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

same as above

@roigcarlo
Copy link
Member

To me it looks good (can't approve because of the commits).

Maybe if we merge with the master it compiles the nightlies, seems that more recent branches have the problem fixed.

@sunethwarna
Copy link
Member Author

I checked the latest nighly build log, the same tests are still failing (and it is 7 hours old). Do you think I should merge master?

@philbucher
Copy link
Member

nightly fails pretty much all the time, there is always sth not working :)
Don't worry if your stuff works

philbucher
philbucher previously approved these changes Jun 19, 2020
View reviewed changes
@sunethwarna sunethwarna changed the title [Ci] Adding statistics application [Ci] Getting ready statistics application for CI integration Jun 19, 2020
@sunethwarna sunethwarna changed the title [Ci] Getting ready statistics application for CI integration [StatApp] Getting ready statistics application for CI integration Jun 19, 2020
@sunethwarna sunethwarna requested a review from philbucher June 19, 2020 09:34
@sunethwarna sunethwarna merged commit 2339b90 into master Jun 19, 2020
@sunethwarna sunethwarna deleted the ci/add_statistics_application branch June 19, 2020 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philbucher philbucher philbucher approved these changes

@roigcarlo roigcarlo Awaiting requested review from roigcarlo

Assignees

@sunethwarna sunethwarna

Labels
Applications Continuous Integration related to Travis, Appveyor, ... Linux Windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sunethwarna @philbucher @mpentek @roigcarlo