Adding `get_tagged_indices` to mphys wrapper

timryanb commented 10 months ago

Purpose

This is related to https://github.com/OpenMDAO/mphys/pull/158 and https://github.com/smdogroup/funtofem/pull/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)
[x] 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

codecov[bot] commented 10 months ago

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%)`	:arrow_down:
openaerostruct/mphys/utils.py	`100.00% <100.00%> (ø)`

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

eytanadler commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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.

A-CGray commented 10 months ago

@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 commented 10 months ago

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

timryanb commented 10 months ago

@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 commented 10 months ago

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:

timryanb commented 10 months ago

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:

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

eytanadler commented 10 months ago

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.

mdolab / OpenAeroStruct