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 math function from file #692 #701

Closed
wants to merge 5 commits into from
Closed

Conversation

ezrayst
Copy link
Contributor

@ezrayst ezrayst commented Nov 27, 2020

Describe the PR
Math function is modified to read acceleration time history of an earthquake, thus better to have it from a file instead of simply a json entry.

Related Issues/PRs
Part 1 of #692

Explanation
This design utilizes the available class structures already available. It mirrors the velocity boundary condition, with input being

node_id    dir    velocity_value / acceleration_value

with the variation over time defined in the math_function. With that, the acceleration will be kept at 1.0 in node and math_function defines the ground motion. The advantage is the robustness as we now have acceleration boundary condition as input, and it can be modified easily to allow for dynamic inputs. This method is preferred, but there may be extra double at each constrained node.

math_function can be defined through a csv file as shown here: https://ezrayst.github.io/mpm-doc/#/user/preprocess/input?id=math-functions

Alternative
To have another class, timehistory.h to define acceleration and stress over time. Those can have public functions such as apply_acceleration_time() that can be called in our solver (mpm) and applied directly instead of having a static value in node. This will mean that we have separate json entry called dynamic_input such as follow:

"dynamic_input": {
    "type": "rigid" (or "flexible"),
    "rigid_acceleration_time": "acc_time_filename.csv",
    "flexible_stress_time": "stress_time_filename.csv"
}

Copy link
Contributor

@bodhinandach bodhinandach left a comment

Choose a reason for hiding this comment

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

Looks okay to me, @ezrayst, one minor comment only. A small question: could you let me know which lines assign these math_functions to nodal variables acceleration/velocity/displacement? Or is it coming in the near future? Thanks btw!

include/io/io_mesh_ascii.tcc Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 27, 2020

Codecov Report

Merging #701 (322e0f5) into develop (c9630b3) will increase coverage by 12.82%.
The diff coverage is 41.18%.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop     #701       +/-   ##
============================================
+ Coverage    83.91%   96.73%   +12.82%     
============================================
  Files          169      130       -39     
  Lines        34176    25849     -8327     
============================================
- Hits         28676    25004     -3672     
+ Misses        5500      845     -4655     
Impacted Files Coverage Δ
include/solvers/mpm_base.h 50.00% <ø> (ø)
tests/io/io_mesh_ascii_test.cc 99.58% <ø> (ø)
include/solvers/mpm_base.tcc 75.64% <37.50%> (ø)
include/io/io_mesh_ascii.tcc 74.80% <100.00%> (ø)
...e/cbgeo/project/external/spdlog/details/registry.h
/home/cbgeo/project/src/io/io.cc
/home/cbgeo/project/tests/test_main.cc
...cbgeo/project/external/spdlog/details/null_mutex.h
...home/cbgeo/project/tests/materials/norsand_test.cc
...ome/cbgeo/project/tests/solvers/mpm_scheme_test.cc
... and 293 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9630b3...322e0f5. Read the comment docs.

@ezrayst
Copy link
Contributor Author

ezrayst commented Nov 27, 2020

It should be coming next, but it is briefly described here #692, @bodhinandach

@jgiven100
Copy link
Collaborator

@ezrayst are acceleration time histories typically only 1d?

@ezrayst
Copy link
Contributor Author

ezrayst commented Nov 27, 2020

@jgiven100, in 2D we usually only use the horizontal acceleration (not vertical), so earthquake input will only be in one direction. In 3D, we have two lateral directions.

But maybe that is not your question (I might be reading too much into this). If we need to run horizontal and vertical acceleration time history, we will have math_function_0 for the x axis and math_function_1 for the y axis.

@jgiven100
Copy link
Collaborator

jgiven100 commented Nov 27, 2020

we will have math_function_0 for the x axis and math_function_1 for the y axis

@ezrayst Right, I was thinking about the 3D case. Seperate math functions for the 2 directions understood 👍

Copy link
Contributor

@kks32 kks32 left a comment

Choose a reason for hiding this comment

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

Instead of writing a custom csv reader, it would be better to use https://github.com/ben-strasser/fast-cpp-csv-parser

Also, the input JSON file structure is needed. Please add more details to the PR describing the design and input file structure.

@ezrayst
Copy link
Contributor Author

ezrayst commented Nov 30, 2020

Hi @kks32, two things:

  • The input file and json input are available here: Add documentation for math function mpm-doc#53, so please take a look. The overall design is described in Earthquake MPM #692, thus this PR is just the first point (make it easier and quicker for review).
  • For the parser, do you want to change it for the whole of our code? This PR is consistent currently with the rest of the code. If you want to change the io to use ben-strasser, I could do that too. Would that be a separate PR?

@kks32
Copy link
Contributor

kks32 commented Nov 30, 2020

@ezrayst

  • It's good to have PRs which are self-contained, so adding input details to the PR description helps to understand how the file would look like and the JSON configuration too since mpm-doc is a separate repository. Additionally, RFC Earthquake MPM #692 is inconsistent with the PR:
A time_history.h class is derived from the abstract function_base.h. Usually this math function is used to yield a value between 0 and 1, which will be multiplied by the prescribed traction or velocity. However, here, the prescribed acceleration is 1 where the time_history.h defines the acceleration given the time. In addition, another alternative input file will be implemented to read a file containing time and value (in this case it is acceleration).

No time_history.h is defined in this PR, so adding input files and design here helps.

  • I'd suggest using the csv parser in io, you can just do it for the time-history files.

@ezrayst
Copy link
Contributor Author

ezrayst commented Dec 1, 2020

@kks32 I updated the initial comment, and I will change to CSV within today, hopefully. Thanks!

@ezrayst
Copy link
Contributor Author

ezrayst commented Dec 3, 2020

@kks32, please let me know if you would like to discuss the structure of the code, but currently I have changed to csv file input. I think I see you are concerned about the size of the file - but I think typical earthquake ground motion would be in the order of thousands of entries, not millions. Thanks anyways!

@ezrayst
Copy link
Contributor Author

ezrayst commented Dec 10, 2020

Hi @kks32, do you still have more comments?

Copy link
Contributor

@kks32 kks32 left a comment

Choose a reason for hiding this comment

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

create a folder inside external for csv and then add their license as well. Follow the convention.

@@ -27,6 +27,9 @@
#include "particle.h"
#include "vector.h"

// CSV-parser
#include "csv.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be included in io class not mpm base

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we move json and catch to their own respective folders?

Copy link
Contributor

@kks32 kks32 left a comment

Choose a reason for hiding this comment

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

Could you please add a test that shows you can read the files and get the correct x and fx values?

@@ -762,16 +762,44 @@ bool mpm::MPMBase<Tdim>::initialise_math_functions(const Json& math_functions) {
const std::string function_type =
function_props["type"].template get<std::string>();

// Initiate another function_prop to be passed
auto function_props_update = function_props;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

io_->json_object("mesh")["io_type"].template get<std::string>();
auto reader = Factory<mpm::IOMesh<Tdim>>::instance()->create(io_type);

// Math function is specified in a file, replace function_props_update
Copy link
Contributor

Choose a reason for hiding this comment

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

Read files in io class not in mpm_base. Create a separate class in io or use the read_mesh_ascii to read these files. I'd prefer for now if you use read_mesh_ascii to read the input file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I have tried to use in the beginning, and you requested CSV. Can we agree on which one to use now before I make more changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not asking you to revert back, this is an io operation and reading the CSV should be in the io class as an additional function and not in the mpm_base

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, so still CSV but within read_mesh_ascii. But doesn't it refer to ascii not csv?

// Create a new function from JSON object
auto function =
Factory<mpm::FunctionBase, unsigned, const Json&>::instance()->create(
function_type, std::move(function_id), function_props);
function_type, std::move(function_id), function_props_update);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it called function_props_update?

// Math function is specified in a file, replace function_props_update
if (function_props.find("file") != function_props.end()) {
// Make separate arrays
std::vector<double> xvalues;
Copy link
Contributor

Choose a reason for hiding this comment

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

Having separate vectors makes it decoupled between x and y values, use a vector of arrays instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@kks32
Copy link
Contributor

kks32 commented Dec 10, 2020

@ezrayst This PR is a generic PR for reading x and fx values from a CSV file and has nothing to do with acceleration time histories per se. Please update your PR details accordingly, you may see it will be used for acceleration time histories, but there is no need to discuss how it will be implemented at the node level because that's not done here. Provide details specific to this PR, which is missing (an input JSON file and an example CSV file would help). These are the core details and should be included rather than a link to mpm-doc PR.

@ezrayst
Copy link
Contributor Author

ezrayst commented Dec 16, 2020

Hi @kks32. I will update the PR accordingly then, to just simply update the math_function. I will make a separate PR regarding Earthquake one, but can we discuss this quickly sometime this week?

io_->json_object("mesh")["io_type"].template get<std::string>();
auto reader = Factory<mpm::IOMesh<Tdim>>::instance()->create(io_type);

// Math function is specified in a file, replace function_props_update
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not asking you to revert back, this is an io operation and reading the CSV should be in the io class as an additional function and not in the mpm_base

@kks32
Copy link
Contributor

kks32 commented Jul 15, 2021

#715 supersedes this PR. Closing

@kks32 kks32 closed this Jul 15, 2021
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.

4 participants