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

Adding get_tagged_indices to mphys wrapper #414

Merged
merged 12 commits into from
Jul 28, 2023
Merged

Conversation

timryanb
Copy link
Contributor

Purpose

This is related to OpenMDAO/mphys#158 and smdogroup/funtofem#192 and allows for aerostructural analysis with multiple bodies

Expected time until merged

a 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

@timryanb timryanb requested a review from a team as a code owner July 20, 2023 14:25
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #414 (a4fa1fc) into main (af05b30) will decrease coverage by 0.01%.
The diff coverage is 93.10%.

@@            Coverage Diff             @@
##             main     #414      +/-   ##
==========================================
- Coverage   94.51%   94.50%   -0.01%     
==========================================
  Files         103      103              
  Lines        6420     6445      +25     
==========================================
+ Hits         6068     6091      +23     
- Misses        352      354       +2     
Files Changed Coverage Δ
openaerostruct/mphys/aero_builder.py 86.66% <86.66%> (-0.22%) ⬇️
openaerostruct/mphys/utils.py 100.00% <100.00%> (ø)

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

@eytanadler
Copy link
Collaborator

Any idea what is going on with PETSc? It's happening in other builds too, so I don't think it's a change caused by this PR. Do you know of any updates to dependencies? I was going to take a look but I haven't had time to get around to it yet.

Other than that, this looks fine to me. I don't know the mphys side of things, so maybe @bernardopacini has some input. If not, we can merge it once the PETSc problem is sorted.

@timryanb
Copy link
Contributor Author

@eytanadler this is likely a problem with Cython 3.0 which was released recently released. There seems to be issue mentioning this on the petsc4py repo: https://gitlab.com/petsc/petsc/-/issues/1423. You might want to try pip installing cython 0.29 before installing petsc4py. We had a similar issue on TACS that @A-CGray discovered, he may have some other ideas

@A-CGray
Copy link
Member

A-CGray commented Jul 27, 2023

Yeah PETSc4py is the issue here, we fixed this in our docker builds by pip installing "cython<3.0". In the longer term though, why don't we just run these actions on one of our docker containers?

@timryanb
Copy link
Contributor Author

@eytanadler I tried making the fix in this PR, but there still seems to be issues with codecov: https://github.com/mdolab/OpenAeroStruct/actions/runs/5681691211/job/15398407295?pr=414

@eytanadler
Copy link
Collaborator

Thanks! For some reason that happens periodically. We tried updating the codecov action to the newest but it doesn't always work. I triggered the tests again to see if it'll work the second time.

@eytanadler eytanadler requested review from A-CGray and removed request for bernardopacini July 27, 2023 19:40
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.

Looks good to me. Let's wait on Ali before merging though.

@A-CGray
Copy link
Member

A-CGray commented Jul 27, 2023

@timryanb can you bump the version to 2.7.0 as this PR introduces a new backwards compatible feature. I would do it but can't push to your fork

@timryanb
Copy link
Contributor Author

timryanb commented Jul 27, 2023

I'm happy to do so @A-CGray, though the feature should be backwards-compatible (non-breaking) for the record

@timryanb
Copy link
Contributor Author

timryanb commented Jul 27, 2023

@eytanadler is it alright to do the version bump in this PR or is it easier to do a new PR dedicated to the bump after this PR?

@A-CGray
Copy link
Member

A-CGray commented Jul 27, 2023

I'm happy to do so @A-CGray, though the feature should be backwards-compatible (non-breaking) for the record

Yep, that's why I want to bump the minor version and not the major version. I don't think we have a consistent rule for this stuff but I try to follow the semantic versioning rules:
image

@timryanb
Copy link
Contributor Author

timryanb commented Jul 27, 2023

I'm happy to do so @A-CGray, though the feature should be backwards-compatible (non-breaking) for the record

Yep, that's why I want to bump the minor version and not the major version. I don't think we have a consistent rule for this stuff but I try to follow the semantic versioning rules: image

Ah sorry, misread the "backwards compatible" comment...

@eytanadler
Copy link
Collaborator

Let's lump the minor version change in with #413. Feel free to update the version number here, but let's not make a new release yet.

@eytanadler eytanadler merged commit 5ebabd1 into mdolab:main Jul 28, 2023
9 checks passed
@timryanb timryanb deleted the mphys_tags branch July 28, 2023 17:21
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