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 FLOPS Aero Builder #619

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

xjjiang
Copy link
Contributor

@xjjiang xjjiang commented Dec 5, 2024

Summary

Added a unit test file for FLOPS based aerodynamic builder (20 unit tests).

Note: the unit test file for GASP based aerodynamic builder has to be put on hold because it depends on the consistency of default values in several GASP based components.

Related Issues

  • Resolves #

Backwards incompatibilities

None

New Dependencies

None

@xjjiang xjjiang changed the title Test flops aero builder Test FLOPS Aero Builder Dec 5, 2024
Copy link
Contributor

@jkirk5 jkirk5 left a comment

Choose a reason for hiding this comment

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

I could use a second opinion on the use of _unspecified from another reviewer

@@ -39,7 +37,7 @@
GASP = LegacyCode.GASP
FLOPS = LegacyCode.FLOPS

_default_name = 'aerodynamics'
default_name = 'aerodynamics'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to keep the old name - it marks default_name as a variable intended as private to this file, so other devs know they shouldn't try from aerodynamics_builder import default_name

It's unlikely anyone would ever even want that, but it's good practice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is required by unittest because TestSubsystemBuilderBase has an attribute default_name.

@@ -387,7 +385,7 @@ def get_parameters(self, aviary_inputs=None, phase_info=None):

val = meta['default_value']
if val is None:
val = _unspecified
val = 0.0 # _unspecified
Copy link
Contributor

Choose a reason for hiding this comment

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

_unspecified might actually be useful here? If used consistently throughout the code it could provide a valid use of None as a default, although I'm unsure where that would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unittest does not like it.

self.assertIn(
'val', val, "The dictionaries returned by get_parameters() should have a 'val' key")
'val', val, f"The dictionaries returned by get_parameters() should have a 'val' key for {key}")
Copy link
Member

Choose a reason for hiding this comment

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

wording
---> should have a 'val' for key '{key}'."

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.

3 participants