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

ESP tutorial #152

Merged
merged 12 commits into from
Sep 19, 2022
Merged

ESP tutorial #152

merged 12 commits into from
Sep 19, 2022

Conversation

hajdik
Copy link
Contributor

@hajdik hajdik commented Aug 21, 2022

Purpose

Adding a simple ESP tutorial demonstrating how to use an ESP model with pyGeo and change the design variables

Expected time until merged

1 week

Type of change

  • 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

The new example script should run in a docker image and produce a new CSM file, modified.csm.

Checklist

  • 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

@codecov
Copy link

codecov bot commented Aug 21, 2022

Codecov Report

Merging #152 (de628db) into main (56c5119) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #152   +/-   ##
=======================================
  Coverage   63.75%   63.75%           
=======================================
  Files          47       47           
  Lines       11786    11786           
=======================================
  Hits         7514     7514           
  Misses       4272     4272           
Impacted Files Coverage Δ
pygeo/__init__.py 82.60% <100.00%> (ø)
pygeo/parameterization/DVGeoESP.py 64.88% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hajdik hajdik marked this pull request as ready for review August 30, 2022 15:25
@hajdik hajdik requested a review from a team as a code owner August 30, 2022 15:25
Copy link
Contributor

@yqliaohk yqliaohk left a comment

Choose a reason for hiding this comment

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

Thanks for putting together the tutorial! Looks good overall. I just have two minor questions.

For information on how to install ESP and its dependencies for use in pyGeo, see the :ref:`installation page<install>`.

The FFD approach is sometimes preferred because it is easy to set up and has a lot of freedom in how the design variables can change the geometry.
Unfortunately, the FFD control points are the primary representation of the geometry, and in some cases it is preferred to have both an input geometry and an output geometry in more traditional formats, like CAD models.
Copy link
Contributor

Choose a reason for hiding this comment

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

"are" or "are not"? Or maybe it's just not clear to what you're trying to say here.

doc/install.rst Outdated
-------------
The simplest way to install ESP so that it works with pyGeo is to use the Docker image mentioned above, which will have pyGeo and ESP both installed.
The ESP GUI can then be installed on your local machine for Mac, Windows, and Linux following the instructions in their README to view ESP models.
Our currently supported version is 1.20, which can be installed from the `archive <https://acdl.mit.edu/ESP/archive/>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

this link does not seem to show properly in the built doc?

yqliaohk
yqliaohk previously approved these changes Aug 31, 2022
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.

Looks good! I made a quick commit to fix the script bug and I did not notice you had a fix for it already

@hajdik hajdik merged commit f6ef819 into main Sep 19, 2022
@hajdik hajdik deleted the esp-docs2 branch September 19, 2022 14:43
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.

None yet

3 participants