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

Custom ptsets #89

Merged
merged 7 commits into from
Aug 7, 2023
Merged

Custom ptsets #89

merged 7 commits into from
Aug 7, 2023

Conversation

anilyil
Copy link
Contributor

@anilyil anilyil commented Jul 20, 2023

Purpose

This PR adds capability to set custom pointsets when adding DVGeo to the aero solver. The custom pointsets are provided in a dict where keys are the corresponding family names in the solver and values are the custom kwargs passed with those pointsets. See the relevant ADflow PR: mdolab/adflow#299

Expected time until merged

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

@anilyil anilyil requested a review from a team as a code owner July 20, 2023 18:03
@anilyil anilyil requested review from A-CGray and gawng July 20, 2023 18:03
@anilyil anilyil requested review from hajdik, sseraj and A-CGray and removed request for A-CGray and gawng July 20, 2023 18:04
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #89 (11ffab5) into main (fc237f3) will decrease coverage by 0.03%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main      #89      +/-   ##
==========================================
- Coverage   35.44%   35.42%   -0.03%     
==========================================
  Files          26       26              
  Lines        2584     2586       +2     
==========================================
  Hits          916      916              
- Misses       1668     1670       +2     
Files Changed Coverage Δ
baseclasses/solvers/pyAero_solver.py 19.07% <33.33%> (-0.26%) ⬇️

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

@marcomangano
Copy link
Contributor

LGTM, I opened a separate PR for the flake8 issues. Once that is merged this is good to go, so we can move on to the related ADflow PR.

@@ -95,6 +96,13 @@ def setDVGeo(self, DVGeo, pointSetKwargs=None):
Keyword arguments to be passed to the DVGeo addPointSet call.
Useful for DVGeometryMulti, specifying FFD projection tolerances, etc.

customPointSetKwargs : dict of dicts
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't match the input argument name. But thinking about it, could we separate the specification of the multiple pointsets from their options? e.g have customPointSetFamilies just be a list of the families to put in separate ptsets, and then add a customPointSetKwargs argument to specify the options for each one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then you would need to specify the ptset names twice. This way its the least amount of code duplication when prescribing. I will reword the docstring but basically the ptsetkwargs will be shared across all ptsets (e.g. nIter=200) and then each family gets its custom args that are applied on top. This approach also avoids using kwargs for multiple purposes depending on a list vs a dictionary that is provided. We use that approach a lot but its not clear what the behavior is. This way there is no ambiguity. I will also explain the order these are applied so if the same option is provided in both one overwrites the other.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I'm fine with just the docstring changes

Copy link
Member

Choose a reason for hiding this comment

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

Bump, @anilyil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed some changes. please take a look

@anilyil anilyil requested a review from A-CGray August 7, 2023 18:55
A-CGray
A-CGray previously approved these changes Aug 7, 2023
if pointSetKwargs is None:
self.pointSetKwargs = {}
else:
self.pointSetKwargs = pointSetKwargs
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code avoids mutable defaults, so we should keep it like this.

customPointSetFamilies : dict of dicts
Keyword arguments to be passed to the DVGeo addPointSet call for each surface family,
specified by the keys. The surface families need to be all part of the designSurfaceFamily.
Useful for DVGeometryMulti, specifying FFD projection tolerances, etc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like the three lines above do not emphasize the main purpose of this argument, which is to have subsets of the CFD surface as separate point sets. I would rephrase this to first talk about what the keys are and what they do, then talk about how the values are dictionaries of keyword arguments.

@anilyil anilyil requested a review from A-CGray August 7, 2023 20:04
@A-CGray A-CGray merged commit 5688faa into mdolab:main Aug 7, 2023
9 of 10 checks passed
@A-CGray A-CGray mentioned this pull request Aug 7, 2023
13 tasks
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