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

Support for Numpy 2.1 and OpenMDAO 3.35 #450

Merged
merged 15 commits into from
Jan 17, 2025
Merged

Conversation

sabakhshi
Copy link
Contributor

@sabakhshi sabakhshi commented Dec 15, 2024

Purpose

Adds compatibility for Numpy 2.1 and OpenMDAO 3.35. Breaks backward compatibility with older OpenMDAO versions due to fundamental changes in how B-splines work in OpenMDAO 3.35.

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

Checklist

  • I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • 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

@sabakhshi sabakhshi requested a review from a team as a code owner December 15, 2024 01:57
@sabakhshi sabakhshi requested review from bernardopacini, eytanadler and kanekosh and removed request for bernardopacini December 15, 2024 01:57
Copy link
Collaborator

@eytanadler eytanadler 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 the PR. x_cp_start and x_cp_end need to be added to all spline components that take in the x_interp normalized interpolation locations (I think that’s all of them?), not just the t/c splines.

The testing problem is due to the fact that in the Docker containers being run on Azure, there is a Python package requirements file the constrains the versions of certain packages. This prevents pip from installing the OpenMDAO version we need. I think there are two ways to fix it. Either edit the OpenMDAO version in the Docker repo to edit the containers (this would affect the containers that all repos use) or create a new Python environment within the Docker container where the constraints file is no longer active (this would only affect OpenAeroStruct). Then run the tests with the new environment. Because this spline change requires the latest OpenMDAO and this is specific to OpenAeroStruct, I’m inclined to go with the second option. The first option would require changing to the latest OpenMDAO version in both the stable and latest Docker containers, which is probably undesirable (unless there’s a way to test OpenAeroStruct on only the latest containers, in which case maybe that would work). Let’s see what @kanekosh thinks.

@kanekosh
Copy link
Contributor

I edited the GHA file to remove version constraints on numpy and openmdao (without editing the Docker repo itself), so the build process now installs OM 3.35+. However, it looks like the latest tests still run with numpy 1.25.2 ... should we explicitly test with numpy 2.1? (This would probably require a new Python environment within the Docker container like Eytan said)

@sabakhshi
Copy link
Contributor Author

I fixed the splines for the structural components which greatly reduced the errors for the failed tests. I spoke to Ali about the discrepancies and we determined that they were consistent with the changes in the OpenMDAO spline component. Also most of the aerostruct tests use very coarse meshes making them very sensitive to the thickness related b splines. This is ready for review and merge.

@sabakhshi sabakhshi requested a review from eytanadler January 15, 2025 23:55
Copy link
Collaborator

@eytanadler eytanadler left a comment

Choose a reason for hiding this comment

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

A couple minor docs changes.

I'm not sure how the git merge was done, but I don't think the changes to the main branch from the MPhys 2.0 updates PR should show up in this diff. @kanekosh do you know what happened here? Or maybe this is fine.

README.md Outdated
@@ -94,13 +94,13 @@ Version Information
The oldest and latest versions of the dependencies that we test regularly are the following (other versions may work, but no guarantees):

| Dependency | oldest | latest |
| ------------------ | ------ | ------ |
|--------------------|--------| ------ |
| Python | 3.8 | 3.11 |
| NumPy | 1.20 | latest |
| SciPy | 1.6.0 | latest |
| OpenMDAO | 3.15 | latest |
Copy link
Collaborator

Choose a reason for hiding this comment

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

OpenMDAO lower bound needs to be updated here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -44,7 +44,7 @@ The oldest and latest versions of the dependencies that we test regularly are th
- 1.6.0
- latest
* - OpenMDAO
- 3.15
- 3.35
- latest
* - Matplotlib
- latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

MPhys should be added here just as it is in the README.

@sabakhshi
Copy link
Contributor Author

A couple minor docs changes.

I'm not sure how the git merge was done, but I don't think the changes to the main branch from the MPhys 2.0 updates PR should show up in this diff. @kanekosh do you know what happened here? Or maybe this is fine.

This branch was not up to date with the latest MPhys fixes from Tim Brooks. As result, the Mphys tests wouldn't run and I couldn't verify them with the changes in this PR. I did a git rebase with the main branch in order to add them to this branch. I might not have done that right.

@kanekosh
Copy link
Contributor

kanekosh commented Jan 17, 2025

A couple minor docs changes.

I'm not sure how the git merge was done, but I don't think the changes to the main branch from the MPhys 2.0 updates PR should show up in this diff. @kanekosh do you know what happened here? Or maybe this is fine.

Right, the diff is messed up... I don't know why (maybe because you merged from mphys branch not from the main?) and I'm not sure if the diff will be resolved when merging this PR into main.
I verified all the changes in the main were properly merged into this branch, so that's good.

We can try merging this into main, and hopefully the squash will sort out the diff issue (or not)?

Other than that, this PR looks good to me. Thanks @sabakhshi !

@sabakhshi sabakhshi requested a review from eytanadler January 17, 2025 16:20
eytanadler
eytanadler previously approved these changes Jan 17, 2025
Copy link
Collaborator

@eytanadler eytanadler left a comment

Choose a reason for hiding this comment

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

Sure, may as well try the merge.

Thanks again Big Saf!

@sabakhshi sabakhshi dismissed eytanadler’s stale review January 17, 2025 16:22

The merge-base changed after approval.

@sabakhshi
Copy link
Contributor Author

I have some concerns about merging into main with this. Should I not have used rebase from main to get the fixes from the last PR?

@sabakhshi sabakhshi requested a review from eytanadler January 17, 2025 16:23
@eytanadler
Copy link
Collaborator

I would've thought rebase would be fine. But, GitHub also seems to have concerns because it instantly rejected my approval of the PR.

eytanadler
eytanadler previously approved these changes Jan 17, 2025
@sabakhshi sabakhshi dismissed eytanadler’s stale review January 17, 2025 16:27

The merge-base changed after approval.

kanekosh
kanekosh previously approved these changes Jan 17, 2025
@kanekosh kanekosh dismissed their stale review January 17, 2025 16:36

The merge-base changed after approval.

@eytanadler eytanadler merged commit b5ac877 into mdolab:main Jan 17, 2025
9 checks passed
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.

3 participants