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

Move mach wrappers out #98

Merged
merged 7 commits into from
Apr 8, 2022
Merged

Move mach wrappers out #98

merged 7 commits into from
Apr 8, 2022

Conversation

anilyil
Copy link
Collaborator

@anilyil anilyil commented Mar 11, 2022

This PR moves the wrappers for ADflow, pyGeo, and RLT to their respective repositories. See the related PRs:
mdolab/adflow#196
mdolab/pygeo#120
https://github.com/mdolab/rlt/pull/12

RLT is private currently, and it also does not work with the latest version of TACS so more fixes are needed on that repo.

This PR also moves the examples that use MACH tools under the relevant example sub-directories.

@anilyil
Copy link
Collaborator Author

anilyil commented Mar 11, 2022

@kejacobson do I need to modify anything else on top of the changes here?

@joanibal can you also please check if the examples and tests run?

@bernardopacini we can either move the DAFoam stuff out of this repo in this PR or create another PR. Let me know what you guys want to do. As a reviewer, you should be able to push to my branch.

Copy link
Collaborator

@kejacobson kejacobson 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 to me.

@kejacobson
Copy link
Collaborator

After this PR, we should be able to close or move the following issues to their parent repos: #54, #61, #62, #69

@anilyil
Copy link
Collaborator Author

anilyil commented Mar 13, 2022

Forgot about the issues. will start moving them

@bernardopacini
Copy link
Collaborator

I just updated the DAFoam side of this (removing the builder and updating the tests). @friedenhe is it okay with you if we do this in this PR?

@joanibal
Copy link
Collaborator

@anilyil I can confirm that the tests run after updating the ADflow side of things

Copy link
Collaborator

@bernardopacini bernardopacini left a comment

Choose a reason for hiding this comment

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

This all looks good to me, but I think we should standardize the naming between the tools. TACS and DAFoam are currently using <package>_mphys while ADflow and pyGeo are just mphys. Also, I think TACS has it in a sub-package / sub-directory if I am not mistaken.

On a separate naming convention note, should we specify that these tests are tool specific? For example test_aerostruct is actually the test for ADflow + TACS aerostructural analysis. Should we be more descriptive?

@@ -29,3 +29,6 @@ dist/
tests/input_files/*.xyz

.ropeproject

# n2s that might be generated under examples
examples/**/*.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to do the same for the tests? I find that I have many n2 diagrams in the tests directories too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, that is fine for me. Those N2s are used for the documentation I think but we are not tracking them so might be a good idea to add them to gitignore

@anilyil
Copy link
Collaborator Author

anilyil commented Mar 23, 2022

The pyGeo PR is just merged: mdolab/pygeo#120 We also have a decent method to install the mphys wrappers in the repo packages, without actually importing them at every import. See how the packages list is handled in pyGeo: https://github.com/mdolab/pygeo/blob/main/setup.py#L25 Then in the actual pyGeo init, we dont import the mphys builder: https://github.com/mdolab/pygeo/blob/main/pygeo/__init__.py but we import it in the init under the mphys folder within the pygeo folder: https://github.com/mdolab/pygeo/blob/main/pygeo/mphys/__init__.py This way, the import

from pygeo.mphys import OM_DVGEOCOMP

works regardless if the install was in place or in system packages. Plus, the mphys related stuff is not imported by default so an environment without openmdao can import other pyGeo classes just fine. We are using the same solution with the adflow wrapper. @timryanb tagging you here in case this can help with TACS imports.

Copy link
Collaborator

@bernardopacini bernardopacini 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 to me.

@anilyil
Copy link
Collaborator Author

anilyil commented Apr 8, 2022

The ADflow (mdolab/adflow#196) and pyGeo (mdolab/pygeo#120) PRs were merged. The RLT PR is still open but we also need to update the RLT code to work with the version of TACS we use with MPhys, so that can be done later. I will now merge this PR that moves the mach examples under the correct directories, and will create a new PR that fixes the inconsistencies in model orientation for the mach examples.

@anilyil anilyil merged commit 07fb6bb into OpenMDAO:main Apr 8, 2022
@anilyil anilyil deleted the move_mach_tools branch April 8, 2022 07:04
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.

4 participants