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

Test geom builders #597

Merged
merged 28 commits into from
Nov 13, 2024
Merged

Test geom builders #597

merged 28 commits into from
Nov 13, 2024

Conversation

xjjiang
Copy link
Contributor

@xjjiang xjjiang commented Nov 4, 2024

Summary

  • Created two unit tests: the GASP based geom subsystem builder and the FLOPS based geom subsystem builder.
  • Changed the default value of Aircraft.HorizontalTail.ASPECT_RATIO to 4.75 in subsystems/geometry/flops_based/characteristic_lengths.py
  • Changed the default value of Aircraft.Wing.FOLDED_SPAN to 25 in subsystems/geometry/flops_based/non_dimensional_conversion.py
  • Changed the default value of Aircraft.HorizontalTail.TAPER_RATIO to 0.352 in subsystems/mass/flops_based/horizontal_tail.py
  • In get_parameters() method of subsystems/geometry/geometry_builder.py, excluded __dict__ from Aircraft.Nacelle.__dict__ because its value is Nacelle object which can not be handled by mappingproxy.
  • Created two unit test files: test_flops_mass_builder.py and test_gasp_mass_builder.py in subsystems/geometry/test.

Related Issues

  • Resolves #

Backwards incompatibilities

None

New Dependencies

None

Copy link
Member

@Kenneth-T-Moore Kenneth-T-Moore left a comment

Choose a reason for hiding this comment

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

I'm not too big a fan of changing the openmdao defaults values in some geo components just so the unit test can be run without calling set_input_defaults manually. Still, this is only a problem because some components currently use those defaults. We should come up with a more uniform policy for setting them -- maybe all 0 or 1, to make it obvious if they were forgotten in the csv. Until then, I will approve the PR.

@xjjiang
Copy link
Contributor Author

xjjiang commented Nov 5, 2024

I'm not too big a fan of changing the openmdao defaults values in some geo components just so the unit test can be run without calling set_input_defaults manually. Still, this is only a problem because some components currently use those defaults. We should come up with a more uniform policy for setting them -- maybe all 0 or 1, to make it obvious if they were forgotten in the csv. Until then, I will approve the PR.

Agree.

  • In testing subsystem builders, there is no set_input_defaults available. Otherwise, I would have used it.
  • Software development has a basic principle that code and data be separated. But We have data everywhere. Since I am sure why we have those non-zero default values, I was worried that some non-zero default values are used somewhere and zero default values somewhere else. This inconsistency is not accepted by the builders.
  • I agree that we should be consistent to set all 0 or 1.
  • I tried to set those default values to 0 and it breaks some tests. Apparently, some tests depend on default values. If it is an problem that we should address, it should be a separate issue.

@crecine
Copy link
Contributor

crecine commented Nov 7, 2024

  • In testing subsystem builders, there is no set_input_defaults available. Otherwise, I would have used it.

can we use set_value instead of set_input_default for testing the subsystem builders?

@xjjiang
Copy link
Contributor Author

xjjiang commented Nov 8, 2024

  • In testing subsystem builders, there is no set_input_defaults available. Otherwise, I would have used it.

can we use set_value instead of set_input_default for testing the subsystem builders?

I tried. It does not work.

xjjiang and others added 3 commits November 12, 2024 17:05
Set use+both+geometries=False and add code_origin=FLOPS. Give it a try.

Co-authored-by: crecine <[email protected]>
Some change as earlier.
same as earlier update.
@jkirk5
Copy link
Contributor

jkirk5 commented Nov 13, 2024

I made an issue to capture the discussion on uniform default values (#605)

@jkirk5 jkirk5 added this pull request to the merge queue Nov 13, 2024
Merged via the queue into OpenMDAO:main with commit 07ca01e Nov 13, 2024
6 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.

4 participants