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

setup.py install #56

Merged
merged 14 commits into from
Jun 16, 2020
Merged

setup.py install #56

merged 14 commits into from
Jun 16, 2020

Conversation

joanibal
Copy link
Collaborator

Purpose

Change the structure of the repo to use the setup.py method of installation.
This will make it easier to install environment specific packages and specify dependencies.

One can install the repo now by doing
make (if applicable) and pip install .

Type of change

  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)

Checklist

  • I have run unit and regression tests which pass locally with my changes

@joanibal joanibal requested a review from a team as a code owner June 12, 2020 14:06
@joanibal joanibal requested review from anilyil, sseraj, ewu63 and eirikurj and removed request for anilyil and sseraj June 12, 2020 14:06
Copy link
Collaborator

@ewu63 ewu63 left a comment

Choose a reason for hiding this comment

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

Similar to idwarp, the complex f2py Makefile needs to update the location where the .so file should be copied to.

@ewu63
Copy link
Collaborator

ewu63 commented Jun 12, 2020

The complex tests need to have the imports changed, mirroring what you did for the real tests.

Actually, both real and complex imports should be changed:

  • no sys.path modification needed
  • I think you can directly do from adflow import ADFLOW and from adflow import ADFLOW_C, since they should be provided by the __init__.py file.
  • the warning about import strategy can be finally removed, since we will now be importing things "correctly"

Copy link
Collaborator

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

Are the included dependencies the minimum required to run the solver only and not the full set of regression tests?

@ewu63
Copy link
Collaborator

ewu63 commented Jun 15, 2020

Correct, I think we should do optional dependencies for idwarp eventually, but it's okay for now. Once we move over to testflo we can also skip tests based on what's installed.

sseraj
sseraj previously approved these changes Jun 15, 2020
Copy link
Collaborator

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

That makes sense

@joanibal joanibal requested a review from ewu63 June 15, 2020 20:16
@joanibal
Copy link
Collaborator Author

@nwu63 Are you happy with the import strategy now?

@ewu63
Copy link
Collaborator

ewu63 commented Jun 15, 2020

Yes I am.

@joanibal
Copy link
Collaborator Author

could you approve the review then?

@eirikurj eirikurj merged commit a6d0ac0 into mdolab:master Jun 16, 2020
marcomangano pushed a commit to marcomangano/adflow that referenced this pull request Aug 19, 2020
Co-authored-by: Neil Wu <neilwu@umich.edu>
Co-authored-by: Neil Wu <neilwu0626@gmail.com>
Co-authored-by: Eirikur Jonsson <36180221+eirikurj@users.noreply.github.com>
@joanibal joanibal deleted the setup branch March 28, 2022 20:41
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