-
Notifications
You must be signed in to change notification settings - Fork 9
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
New tutorial #57
New tutorial #57
Conversation
A couple of comments:
That's all I can think of at the moment. |
Thanks for the comments @DavidAKopriva ! I will update and simplify the second part of the tutorial from your suggestions. I also need to investigate the case sensitivity (or seemingly the lack of it) for the boundary name |
Thanks for writing the tutorial. Yes, HOHQMesh calls ToLower(str) on the name before testing if it is equal to "symmetry", so it doesn't matter what case one uses. We should think if that should be done for all keywords, but it doesn't matter using HOHQmesh.jl because the control file written out will have the correct case. |
Thanks for the information. I assumed that there was something under hood helping with the case sensitivity, but I was only checking inside the Julia code. Avoiding case sensitivity on all the keywords could help make the control files more " idiot proof", but we can discuss this further in a different HOHQMesh PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
+ Coverage 98.80% 98.81% +0.01%
==========================================
Files 21 21
Lines 1840 1863 +23
==========================================
+ Hits 1818 1841 +23
Misses 22 22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Make some corrections and editorial cahnges.
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.
Changes as is are fine, but I think the renameCurve! procedure description should be added to the API documentation, probably in the model API since it is a model operation. I don't think that it is in there yet.
I added the |
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.
Just a few comments/suggestions that came up when perusing this PR
Make changes to indicate that renameCurve! will change the name of all curves that have a given name.
Fix a typo and modify one sentence.
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 the changes are fine, and I approve modulo the windows fix.
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.
LGTM
…s issues on Windows
…esh.jl into symmetric_tutorial
aa3c03b
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 see there is no check on whether or not a file exists when rm() is called in the remove_mesh! procedure. I would guess that an error message will be written, and after thinking about it, that's OK.
@sloede Are we good to merge? Once this is done I can release a new version of HOHQMesh.jl |
This adds an additional tutorial and discussion of the newly released feature in HOHQMesh that one can define a straight about which a mesh can be reflected.