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 infill density benchmark #4716
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.
These are the comments I made last night (so they are for an older version). I am submitting that review for now and then I'll have a look at the new version.
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 is great, thank you for contributing this! If you have time, it would also be great if you could add a short description in the manual, but if not, that's fine as well. I mostly have a few suggestions for documentation, but otherwise this looks great!
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.
@danieldouglas92 - Nice work, this is a really nice contribution! I only have minor questions and comments, and overall the PR is in great shape.
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.
I think there were some comments from my first review you did not address (maybe because they were listed as outdated), I copied them here again. Otherwise this looks good!
I haven't looked at the manual text yet (since it does not build yet) and I can do this next after you rebase.
The only other point I remembered: We usually have a test case for each benchmark. You don't need to copy the .prm file, but you can instead include your benchmark prm file in a test and then set the end time to zero (you can see an example of that in sol_kz_2_project_q1_only_visc.prm). The point is to make sure that even if we make future changes in ASPECT, the tester will still execute the files you added and we will notice if it breaks.
7a33ca5
to
4329d1f
Compare
/rebuild |
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.
Great, looks good! Thank you for contributing this!
This is ready to merge once the tester is finished.
@naliboff You still have to approve before we can merge this. |
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.
@danieldouglas92 - Nice work, ready to merge!
Pull Request Checklist. Please read and check each box with an X. Delete any part not applicable. Ask on the forum if you need help with any step.
Describe what you did in this PR and why you did it.
Before your first pull request:
For all pull requests:
For new features/models or changes of existing features: