-
Notifications
You must be signed in to change notification settings - Fork 188
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 script to plot power monitors #4663
Add script to plot power monitors #4663
Conversation
39b2200
to
8c51374
Compare
fig, axes = plt.subplots(nrows=1, | ||
ncols=num_plots, | ||
figsize=(num_plots * 4, 4), |
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.
If you have more than two plots, this seems like it'll just create a super wide image/pdf. Is that intended? What about rows of 2 plots?
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.
You choose the block groups on the command line, so you probably won't make plots with more than 3 or 4 block groups. Also, the single row helps comparing the truncation errors side-by-side. So I'd keep the single row for now. If this becomes an issue we can think of ways to organize the plot layout better.
for d, modes_dim in enumerate(all_modes): | ||
ax.semilogy(modes_dim, **props_dim[d], zorder=30 + d) |
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.
Why 30? Just a large number?
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.
yea for the zorder only relative numbers matter
help=("Observation step number. " | ||
"Specify '-1' for the last step in the file. " | ||
"Mutually exclusive with '--time'.")) |
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.
How hard would it be to use last
or Last
to specify the last step rather than -1
. When I was looking at the command, the -1
looked like its own option unrelated to --step
(because of the hyphen/negative).
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.
done
@click.option("--block", | ||
"-b", | ||
"block_or_group_names", | ||
multiple=True, | ||
help=("Names of blocks or block groups to analyze.")) |
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.
Does the multiple=True
propagate to the help text? If not, could you add that to the help text?
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.
Looks good. You can squash
6ad9ede
to
3c05a66
Compare
Nice! Looks good to me @nilsvu |
3c05a66
to
7c0555e
Compare
@sxs-collaboration/spectre-core-devs could one of you sign off on this? The CylindricalBBH domain test timed out so I restarted the one build. |
|
||
|
||
if __name__ == "__main__": | ||
plot_power_monitors_command(help_option_names=["-h", "--help"]) |
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.
When I run this with -h
what general info do I get? I find it useful to have a description of what the code does in addition to the options.
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.
Thanks for catching this, I forgot to add a docstring here. See fixup.
@click.option("--output", | ||
"-o", | ||
type=click.Path(writable=True), | ||
help=("Output text file")) |
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 is saved into this text file? And where are is the image with the PM plots saved?
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.
The help text was just wrong, thanks for catching this as well.
Regenerate the vol data on the current branch, so that it includes block names.
ea82598
to
58ee830
Compare
Merging because all that happened was a squash & rebase |
Proposed changes
Creates plots like this:
Invoke the script like this:
$ spectre plot-power-monitors ElasticityVolume0.h5 -d VolumeData \ -b InnerCube -b Wedges -y 'Displacement_*' -o plot.pdf
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments