Closed kanekosh closed 2 years ago
Merging #379 (8fedc76) into main (95d0a0f) will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## main #379 +/- ##
=======================================
Coverage 96.61% 96.61%
=======================================
Files 93 93
Lines 5912 5912
=======================================
Hits 5712 5712
Misses 200 200
Impacted Files | Coverage Δ | |
---|---|---|
geometry/utils.py | 53.71% <0.00%> (ø) |
:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more
I will review this PR by Tuesday the 15th
Looks good to me!
Actually I broke the wingbox table and I'm fixing it right now. Thanks @eytanadler for the nice suggestions! @jexalto let me know if you have any comments (because I believe you recently went through these stuff).
My bad, I realized after I did it that the approval was a bit premature.
Fixed the wingbox table, ready for review now.
Distinction between "Advanced features" and "User reference". Could you clarify a bit on this?
"Advanced features" are advanced capabilities that are not explained/introduced in the aero/struct/aerostructural optimization pages. "User reference" is not really about the OAS features/capabilities, but it is a list of other useful information. In that sense, "Geometry creation and manipulation" and "Custom problem setup" might fit better in "Advanced Feature", I'm not sure.
I don't have a strong preference, I'm open to a better section naming or grouping.
Should link the page Aerostructural Optimization with Wingbox to the surf dict /mesh dict documentation (This should probably be fixed here)
I didn't put the link there because Sham already wrote a pretty detailed explanation of all keys in sentences, and I'd like users to read Sham's explanation rather than the new surface dict page. I actually copied&pasted a lot from Sham's explanations there to create the tables.
Sham's email should probably be removed
I'll ask him (and John - his umich email in setup.py) if they'd like us to remove it.
Citations can be handled better by using Sphinx directives A lot of math stuff can be rendered using Sphinx directives
Good points, to be addressed someday in the future (not this time).
"Advanced features" are advanced capabilities that are not explained/introduced in the aero/struct/aerostructural optimization pages. "User reference" is not really about the OAS features/capabilities, but it is a list of other useful information. In that sense, "Geometry creation and manipulation" and "Custom problem setup" might fit better in "Advanced Feature", I'm not sure.
I agree, I think those two sections should go into Advanced Features. I also think the contributing page should be its own section.
I didn't put the link there because Sham already wrote a pretty detailed explanation of all keys in sentences, and I'd like users to read Sham's explanation rather than the new surface dict page. I actually copied&pasted a lot from Sham's explanations there to create the tables.
I see what you mean, but I think it's important to have a single, authoritative source for that information. IMO that should be the dedicated page, and other places should just link to it. This ensures that in the future, when we change anything, that is updated correctly since there is no duplicate information. I also think the comments in the code (inside the dictionary) should be removed now that we have good documentation for it.
geometry creation and manipulation
and custom problem setup
to advanced features
section.how to contribute
page to the root (directly under index.rst).I see your point that we should not have duplication, but I still don't want to remove Sham's explanation and inline comments. It'd be inconvenient for users to open two pages and go back and forth, rather than just a single page or python file that we have now. I think that inconvenience is bigger than (probability of changing the surface dict entries in the future) * (problems that would cause)
.
I see your point that we should not have duplication, but I still don't want to remove Sham's explanation and inline comments. It'd be inconvenient for users to open two pages and go back and forth, rather than just a single page or python file that we have now. I think that inconvenience is bigger than (probability of changing the surface dict entries in the future) * (problems that would cause).
Fair point, I'm happy with the repo as is. In our MACH-related documentation, what we do is we hyperlink (via sphinx) to the specific options. I don't think we can do that yet without some substantial overhaul of options documentation, so I'm happy with this.
Purpose
I did the following documentation updates:
mesh_dict
andsurface_dict
entries (not a complete list, but better than having nothing). Please seemesh_surface_dict.rst
. Closes #350 .mesh_dict
ingeometry/utils.py
. I removed some entries that were never used, and added a default value fornum_twist_cp
, which is required for CRM but we didn't have a default value.The documentation structure is now like below. I think it's better than the current one, but I appreciate any suggestions!
Expected time until merged
Not urgent, hopefully by March 18.
Type of change
Checklist
flake8
andblack
to make sure the code adheres to PEP-8 and is consistently formatted