mdolab / pygeo

pyGeo provides geometric design variables and constraints suitable for gradient-based optimization.
https://mdolab-pygeo.readthedocs-hosted.com/en/latest/?badge=latest
Apache License 2.0
122 stars 54 forks source link

Added kwargs to MPhys APIs #248

Closed friedenhe closed 2 months ago

friedenhe commented 2 months ago

Purpose

Added **kwargs to a few MPhys APIs to use more options from the original pyGeo APIs

Expected time until merged

This is a very minor change. So it should be easy to merge.

Type of change

Testing

Checklist

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 65.47%. Comparing base (4c762ab) to head (15cd55f).

Files Patch % Lines
pygeo/mphys/mphys_dvgeo.py 0.00% 10 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #248 +/- ## ======================================= Coverage 65.47% 65.47% ======================================= Files 47 47 Lines 12265 12265 ======================================= Hits 8030 8030 Misses 4235 4235 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ArshSaja commented 2 months ago

This is fine by me, but I say we first wait until the people working on recent MPhys changes merge. @lamkina @ArshSaja is this okay with the ongoing MPhys changes?

I am going to look at the MPhys beta changes in detail during the workshop for ADflow. I will be able to look at this together with ADflow.

friedenhe commented 2 months ago

I just pushed a new commit to add **kwargs to the nom_addGlobalDV API. This is to allow mphys_pygeo to use pyGeo's arguments such as volumes and config

lamkina commented 2 months ago

I realize it's a bit more work, but I would prefer including the arguments and defaults explicitly instead of **kwargs. This is nicer for people that use feature rich code editors because it will show all the arguments in the MPhys layer rather than **kwargs which will require tracking down the arguments in the DVGeo layer as well.

friedenhe commented 2 months ago

@lamkina Sounds good. If @anilyil, @marcomangano, and @ArshSaja have no objection to this, I can push a new commit to explicitly add all pyGeo's arguments to mphys_pygeo (most of them are already there, but there are still a few missing). I can also copy the descriptions of these arguments to mphys_pygeo.

anilyil commented 2 months ago

I agree with @lamkina that this would be a better solution. Otherwise, mismatch in argument names can cause a lot of headaches. It is better to explicitly state all of them. I originally did not recommend this to avoid causing extra work.

@friedenhe I am fine with your suggested change and can review it quickly once you push

friedenhe commented 2 months ago

@anilyil I removed kwargs and explicitly added arguments for some FFD-based APIs. I am not very familiar with the VSP and ESP APIs, so I did not change them; they still have kwargs.

lamkina commented 2 months ago

Thanks @friedenhe , this is a nice update. I will open an issue for the ESP and VSP arguments so we can get that done in another PR as well.