Skip to content
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

Merged
merged 27 commits into from
Jun 25, 2024
Merged

New tutorial #57

merged 27 commits into from
Jun 25, 2024

Conversation

andrewwinters5000
Copy link
Member

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.

@DavidAKopriva
Copy link
Collaborator

DavidAKopriva commented Jun 14, 2024

A couple of comments:

  1. HOHQMesh (rather than fortran) is case insensitive for the "symmetry" keyword. (Hmm, but all other keywords are case sensitive.
  2. You should only need to change the curve names rather than re-construct the model. e.g. setCurveName!(line9,"B9") and setCurveName!(line2,"symmetry") and then regenerate.
  3. In the second tutorial, the boundaries O.4 and O.8 will be B3 and B3_R and similarly for O.8. They become internal boundaries, rather than internal parts of the mesh. To do a fully symmetric mesh without internal boundaries, O.4 and O.8 need their names to also be set to "symmetry". This flexibility lets one define internal boundaries, as requested in issue 49 of HOHQMesh.
  4. You need to delete the mesh before re-generating. (I think.) I suppose the default behavior should be that when generate_mesh is called the mesh, if it exists, should be deleted. Does it do that? I don't remember.

That's all I can think of at the moment.

@andrewwinters5000
Copy link
Member Author

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 "symmetry". I tried a the three capitalization strategies for "Symmetry", "SYMMETRY" and sYmMeTrY" and a valid mesh was generated for all of them.

@DavidAKopriva
Copy link
Collaborator

DavidAKopriva commented Jun 15, 2024

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.

@andrewwinters5000
Copy link
Member Author

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.

@andrewwinters5000
Copy link
Member Author

Better boundary name figures

Screenshot 2024-06-19 at 14 21 30
Screenshot 2024-06-19 at 14 21 53
Screenshot 2024-06-19 at 14 22 17
Screenshot 2024-06-19 at 14 22 44

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.81%. Comparing base (e044f3b) to head (526a29c).
Report is 1 commits behind head on main.

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              
Flag Coverage Δ
unittests 98.81% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@DavidAKopriva DavidAKopriva left a 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.

@andrewwinters5000
Copy link
Member Author

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 renameCurve! to the API and updated the remover_mesh! function to also delete the plot, control, and stats files.

Copy link
Member

@sloede sloede left a 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

docs/src/tutorials/symmetric_mesh.md Outdated Show resolved Hide resolved
docs/src/tutorials/symmetric_mesh.md Outdated Show resolved Hide resolved
docs/src/tutorials/symmetric_mesh.md Outdated Show resolved Hide resolved
docs/src/tutorials/symmetric_mesh.md Show resolved Hide resolved
@andrewwinters5000
Copy link
Member Author

New figures now that that symmetry name has changed to :symmetry

Screenshot 2024-06-21 at 23 43 08 Screenshot 2024-06-21 at 23 43 26 Screenshot 2024-06-21 at 23 43 47 Screenshot 2024-06-21 at 23 44 05

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.
DavidAKopriva
DavidAKopriva previously approved these changes Jun 21, 2024
Copy link
Collaborator

@DavidAKopriva DavidAKopriva left a 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.

sloede
sloede previously approved these changes Jun 22, 2024
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sloede sloede enabled auto-merge (squash) June 22, 2024 03:49
Copy link
Collaborator

@DavidAKopriva DavidAKopriva left a 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.

@andrewwinters5000
Copy link
Member Author

@sloede Are we good to merge? Once this is done I can release a new version of HOHQMesh.jl

@sloede sloede merged commit ee72a13 into main Jun 25, 2024
10 checks passed
@sloede sloede deleted the symmetric_tutorial branch June 25, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants