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

Prefoil dependency #147

Closed
sseraj opened this issue Jul 29, 2022 · 3 comments · Fixed by #148
Closed

Prefoil dependency #147

sseraj opened this issue Jul 29, 2022 · 3 comments · Fixed by #148
Assignees
Labels
bug Something isn't working

Comments

@sseraj
Copy link
Collaborator

sseraj commented Jul 29, 2022

Description

#141 added a hard prefoil dependency but setup.py was not updated to account for this. All tests that import pyGeo fail if you do not also have prefoil installed (which includes some ADflow tests).

At the very least, prefoil should be added in install_requires. However, I don't see much use in making everyone who wants to use pyGeo install prefoil. I would prefer if DVGeometryCST was imported like the other classes that have non-standard dependencies:

pygeo/pygeo/__init__.py

Lines 11 to 22 in d786ca8

try:
from .parameterization import DVGeometryVSP
except ImportError:
pass
try:
from .parameterization import DVGeometryESP
except ImportError:
pass
try:
from .parameterization import DVGeometryMulti
except ImportError:
pass

Similarly, if DVGeometryCST cannot be imported, we should skip its tests.

We can use this issue to discuss what the best approach is.

@sseraj sseraj added the bug Something isn't working label Jul 29, 2022
@eytanadler
Copy link
Collaborator

I'm happy to do the latter where DVGeometryCST is imported using the try/except block. Shall I make a PR with the change and we can discuss from there?

@sseraj
Copy link
Collaborator Author

sseraj commented Jul 29, 2022

Sounds good. Thanks!

@eytanadler
Copy link
Collaborator

Ok, working on it now. Sorry about the oversight!

@eytanadler eytanadler mentioned this issue Jul 29, 2022
12 tasks
@sseraj sseraj linked a pull request Jul 31, 2022 that will close this issue
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants