-
Notifications
You must be signed in to change notification settings - Fork 35
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
Updating documentation to be more explicit in simulation block #70
base: master
Are you sure you want to change the base?
Conversation
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.
What about adding the unit names somewhere. Or is it better to address it in a separate PR closing #66?
<td>Default</td> | ||
</tr> | ||
<tr> | ||
<td>tstart</td> |
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.
rename to start_time for consistency with the reports sections
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.
@apdavison, @salvadord, @pgleeson, are you OK with the suggested renaming?
<td>0.0</td> | ||
</tr> | ||
<tr> | ||
<td>tstop</td> |
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.
rename to end_time for consistency
<td></td> | ||
</tr> | ||
<tr> | ||
<td>block_step_size</td> |
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.
This name is not very clear. If I understand the description correctly I'd rename it to "reporting_skipped_frames" , "skipped_frames", "skip_report_frames", "reporting_start_frame"
I'd also use double for all timestamps, but I think that this is irrelevant for json anyway (isn't number the actual typename in any case?)
A fix to issue #52 (and #22), explicitly adding the definition of "dL" parameter in the "run" block of the simulation config.
I went a head and added a table for all the attributes in the block.