Skip to content

Implement postprocess #84

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Implement postprocess #84

wants to merge 10 commits into from

Conversation

tsarikahin
Copy link

@tsarikahin tsarikahin commented Apr 26, 2023

Description

In this MR followings are aimed:

  • Introduce the second argument obtained by the command line to describe the result prefix
  • Introduce post process functions
  • Adapt mirco_evaluate to return necessary variables, contact force, coordinates, and so on
  • Adapt integration tests according to the second argument

Interested Parties / Possible Reviewers

@RShaw026 @isteinbrecher

@tsarikahin tsarikahin requested a review from RShaw026 April 26, 2023 18:14
@isteinbrecher
Copy link

@tsarikahin can you please describe with a few words what the proposed changes do?

@@ -21,10 +21,11 @@
#include "mirco_nonlinearsolver.h"


void MIRCO::Evaluate(double& pressure, double Delta, double LateralLength, double GridSize,

Choose a reason for hiding this comment

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

This will break the MIRCO integration in BACI. @tsarikahin please have a look together with @RShaw026 to assess the effect of this changes also on the BACI integration. If you make a change that breaks the API for BACI, then you need to also bring those changes to BACI.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I already talked to Rishav about that and he mentioned all those challenges. But I believe it is a necessary step for the future since in mirco_evaluate, no information is returned to the main file.

@tsarikahin
Copy link
Author

@RShaw026 it should be fine now.

@RShaw026
Copy link
Contributor

@tsarikahin Thanks Tarik. Since I need this interface for the Evaluate call in BACI, I will be making slight changes in this branch, and then we can go ahead with the merge request.


const auto start = std::chrono::high_resolution_clock::now();
// following function generates the actual path of the output file.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// following function generates the actual path of the output file.
// Generate the <...> path to the output file

... replace <...> with absolute or relative.

// Initialise Pressure
double pressure = 0.0;
// Number of contact points
int nf = 0;
Copy link
Member

Choose a reason for hiding this comment

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

nf is not a great name for a variable. Why not num_contact_points? Then, one does not need a comment to explain it.

// Number of contact points
int nf = 0;
// Coordinates of the points in contact in the last iteration.
std::vector<double> xvf, yvf;
Copy link
Member

Choose a reason for hiding this comment

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

Not great variabel names

// Coordinates of the points in contact in the last iteration.
std::vector<double> xvf, yvf;
// Contact force at (xvf,yvf) calculated in the last iteration.
std::vector<double> pf;
Copy link
Member

Choose a reason for hiding this comment

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

Not a great name for a variable.

#include <numeric>
#include <vector>

long findClosestIndex(const std::vector<double>& sorted_array, double x)
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need the long type or would int be sufficient?

Comment on lines +43 to +44
// std::cout << ix << "\t" << iy << std::endl;
// std::cout << xvf[i] << "\t" << yvf[i] << "\t" << pf[i] << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// std::cout << ix << "\t" << iy << std::endl;
// std::cout << xvf[i] << "\t" << yvf[i] << "\t" << pf[i] << std::endl;


// Calculate average pressure via total force
auto totalForce = std::accumulate(pf.begin(), pf.end(), 0.0);
double pressure = totalForce / pow(LateralLength, 2);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
double pressure = totalForce / pow(LateralLength, 2);
const double pressure = totalForce / pow(LateralLength, 2);

}
outputForceFile.close();

// Calculate average pressure via total force
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Calculate average pressure via total force
// Calculate average pressure from total force

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.

4 participants