-
Notifications
You must be signed in to change notification settings - Fork 27
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
Adds error plot and colourbars to the animation. Start of a loss plot #112
base: main
Are you sure you want to change the base?
Conversation
The resolution also needs updating as the gifs are quite large now. |
90637b5
to
fad73b2
Compare
This was fixed in the last commit |
The examples don’t appear to have tests, do I need to add some? |
fad73b2
to
12d0aa4
Compare
Signed-off-by: LimitingFactor <[email protected]>
Signed-off-by: LimitingFactor <[email protected]>
Signed-off-by: LimitingFactor <[email protected]>
12d0aa4
to
d99c745
Compare
Hi @cfd1 , thanks for the PR! Is this ready for review? |
@mnabian yes, it's ready for a review please |
No you don't need to add tests for the examples. For documentation, take a look at the README file for the vortex shedding example and see if anything needs to be updated. Thanks |
@@ -217,5 +297,18 @@ def animate(self, num): | |||
frames=len(rollout.graphs) // C.frame_skip, | |||
interval=C.frame_interval, | |||
) | |||
ani.save("animations/animation_" + C.viz_vars[i] + ".gif") | |||
ani.save("animations/animation_" + C.viz_vars[i] + ".gif", dpi=50) |
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.
Can we make the resolution a configurable variable?
@cfd1 thanks again for the PR! It looks good to me, I just left a single comment for you about the resolution. |
/blossom-ci |
Modulus Pull Request
Description
Aims to improve the inference code with an error plot and colourbars.
There is a start of the code to plot the loss over the rollout. Needs cleaning up and generalising.
Checklist
up to date with these changes.
Dependencies
Depends on PR #111