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 expanded feature docs #71

Merged
merged 56 commits into from
Aug 4, 2022
Merged

New expanded feature docs #71

merged 56 commits into from
Aug 4, 2022

Conversation

bbrelje
Copy link
Collaborator

@bbrelje bbrelje commented Mar 2, 2021

Purpose

The feature docs are nonexistent for pygeo. We need feature documents that explain the most common use cases and provide some intutition for how the geometry works, especially FFD

Type of change

What types of change is it?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Docs build correctly

Checklist

Put an x in the boxes that apply.

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@anilyil
Copy link
Contributor

anilyil commented Mar 2, 2021

We should probably add the DVGeoVSP and ESP classes to the auto-generated docs. I am guessing you are on this, if not, let me know and I can add the pages to the documentation.

@marcomangano
Copy link
Contributor

Actually:

Extension error:
Could not import extension embed_code (exception: No module named 'openmdao')

@eirikurj
Copy link
Contributor

eirikurj commented Mar 2, 2021

@bbrelje do we need the openmdao embed_code extension (and the associated code)? Can we not achieve the same with the built-in sphinx literalinclude extensions like we do for our other tutorials e.g.,

.. literalinclude:: ../tutorial/aero/geometry/generate_wing.py
    :start-after: # rst Imports
    :end-before: # rst Airfoil file

@anilyil anilyil self-requested a review March 2, 2021 11:45
@bbrelje
Copy link
Collaborator Author

bbrelje commented Mar 2, 2021

@bbrelje do we need the openmdao embed_code extension (and the associated code)? Can we not achieve the same with the built-in sphinx literalinclude extensions like we do for our other tutorials e.g.,

.. literalinclude:: ../tutorial/aero/geometry/generate_wing.py
    :start-after: # rst Imports
    :end-before: # rst Airfoil file

wait what is this? I've never seen this before

@bbrelje
Copy link
Collaborator Author

bbrelje commented Mar 2, 2021

@anilyil good catch, I can replicate everything with the literalinclude directive so I can remove the openmdao dependency

@ewu63
Copy link
Collaborator

ewu63 commented Mar 2, 2021

@bbrelje do we need the openmdao embed_code extension (and the associated code)? Can we not achieve the same with the built-in sphinx literalinclude extensions like we do for our other tutorials e.g.,

.. literalinclude:: ../tutorial/aero/geometry/generate_wing.py
    :start-after: # rst Imports
    :end-before: # rst Airfoil file

I think @bbrelje wants the code to be executed and the output shown on screen, like what we have for some OpenMDAO-based repos. This avoids having to update the outputs as the code behaviour changes.

@eirikurj
Copy link
Contributor

eirikurj commented Mar 2, 2021

@nwu63 right thats what I though too initially, and maybe that is the ultimate goal here. But it seems that only the code option is used here, which suggest that its only "cropping" and including a specific block of code and thats why I suggested the literalinclude.

@ewu63
Copy link
Collaborator

ewu63 commented Mar 2, 2021

@nwu63 right thats what I though too initially, and maybe that is the ultimate goal here. But it seems that only the code option is used here, which suggest that its only "cropping" and including a specific block of code and thats why I suggested the literalinclude.

I see, yeah that makes sense. There are some difficulties in using embed_code, the main obstacle being that you need the code to be installed and executed on the RTD server, which may be difficult to do especially if it has complex dependencies. The easiest approach would definitely be using literalinclude.

@ewu63
Copy link
Collaborator

ewu63 commented Mar 17, 2021

FYI #72 has been merged @bbrelje

Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

Some minor edits, but I am now pretty confused by the "additional" embedded points included in the cylinder example. What's the idea behind that?

@@ -29,6 +29,11 @@ Examples of geometry design variables might include:
The set of all geometry design variables and their effects on the baseline geometry is called a *parameterization*.
Parameterizations are not unique, and choosing a good one is both an art and a science.

There are two primary approaches to geometry parameterization: generative and deformative.
In pyGeo, we primarily use a free form deformation (FFD) approach, which is deformative.
A deformative parameterization does not require as detailed of a parameterization as a generative approach, such as CAD, does.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rephrase a bit like this, but feel free to further edit:
"There are two primary approaches to geometry parameterization: generative and deformative.
In pyGeo, we primarily use a free form deformation (FFD) approach, which belongs to the latter category.
This approach does not require the parameterization of the baseline geometry (as is the case for a generative approach such as CAD) because we only parameterized the changes with respect to the unperturbed geometry"

Copy link
Contributor

Choose a reason for hiding this comment

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

Re: the additional embedded points: Josh requested showing the FFD embedding volume that can be shown using #133

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm.. I think I am misunderstanding what these blue points represent. Are they the "embedded volume" that's returned after #133? to me they look like an extra "random" pointset and the shaded area is already effective at showing the extent of the FFD volume. We should clarify where they is coming from because we previously refer to the blue triangles that define the cylinder as pointset, and they are the "deformation target".
Hope I am not making things more confusing, we can talk offline

doc/advanced_ffd.rst Outdated Show resolved Hide resolved
joanibal
joanibal previously approved these changes Aug 2, 2022
Copy link
Collaborator

@joanibal joanibal left a comment

Choose a reason for hiding this comment

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

LGTM, merge it!

@marcomangano
Copy link
Contributor

Good job everyone!

@marcomangano marcomangano merged commit 23150ad into mdolab:main Aug 4, 2022
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.

8 participants