Skip to content

Comments

Add documentation for math function#53

Open
ezrayst wants to merge 8 commits intocb-geo:masterfrom
ezrayst:master
Open

Add documentation for math function#53
ezrayst wants to merge 8 commits intocb-geo:masterfrom
ezrayst:master

Conversation

@ezrayst
Copy link
Contributor

@ezrayst ezrayst commented Nov 27, 2020

Documentation for cb-geo/mpm#701

HTML can be seen here: https://ezrayst.github.io/mpm-doc/#/

Copy link
Contributor

@jgiven100 jgiven100 left a comment

Choose a reason for hiding this comment

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

Minor nitpick, otherwise I'm ok

![x_fx](x_fx.png)

### ASCII Math Functions
Math functions can also be specified through an ASCII file, and this is useful when it is rather long such that can be used to define time history of a dynamic earthquake ground motion. The JSON configuration for the math function is:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Math functions can also be specified through an ASCII file, and this is useful when it is rather long such that can be used to define time history of a dynamic earthquake ground motion. The JSON configuration for the math function is:
Math functions can also be specified through an ASCII file, and this is useful when they are rather long such as math functions used to define time history of a dynamic earthquake ground motion. The JSON configuration for the math function is:

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.

Thanks @ezrayst

"id": 1,
"type": "Linear",
"file": "math-function.txt",
"xvalues": [],
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are setting a file, you shouldn't need x and fx values

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

Math function can be specified in the following format:

```
x_0 fx_0
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we go csv instead of tab separated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do so. Will change the main PR first

{
"id": 1,
"type": "Linear",
"file": "math-function.csv"
Copy link
Contributor

Choose a reason for hiding this comment

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

There is inconsistency in naming, here it's called file and above csvfile, please use 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.

Fixed

Math function can be specified in the following format:

```
x_0, fx_0,
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are using a csv parser, could we have a header line?

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

@ezrayst
Copy link
Contributor Author

ezrayst commented Dec 10, 2020

@kks32, do you still have more comments on this?

@kks32 kks32 added the Status: Blocked Status: Blocked label Dec 10, 2020
@kks32
Copy link
Contributor

kks32 commented Dec 10, 2020

Blocked by: cb-geo/mpm#701

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority: Medium Priority: Medium Status: Blocked Status: Blocked Status: Review needed Status: Review needed Type: Feature request Type: Feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants