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

Added kwargs to MPhys APIs #248

Merged
merged 6 commits into from
Jun 26, 2024
Merged

Added kwargs to MPhys APIs #248

merged 6 commits into from
Jun 26, 2024

Conversation

friedenhe
Copy link
Collaborator

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

  • 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

@friedenhe friedenhe requested a review from a team as a code owner June 22, 2024 21:57
Copy link

codecov bot commented Jun 22, 2024

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).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pygeo/mphys/mphys_dvgeo.py 0.00% 10 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #248   +/-   ##
=======================================
  Coverage   65.47%   65.47%           
=======================================
  Files          47       47           
  Lines       12265    12265           
=======================================
  Hits         8030     8030           
  Misses       4235     4235           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

marcomangano
marcomangano previously approved these changes Jun 22, 2024
Copy link
Contributor

@anilyil anilyil left a comment

Choose a reason for hiding this comment

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

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?

@ArshSaja
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

lamkina commented Jun 25, 2024

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
Copy link
Collaborator Author

@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
Copy link
Contributor

anilyil commented Jun 25, 2024

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
Copy link
Collaborator Author

@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
Copy link
Contributor

lamkina commented Jun 26, 2024

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.

@anilyil anilyil merged commit 3404b51 into mdolab:main Jun 26, 2024
11 of 12 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.

5 participants