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

USatm1976Comp now accepts altitude as either geopotential or geodetic #735

Merged
merged 2 commits into from
Apr 7, 2022

Conversation

robfalck
Copy link
Contributor

@robfalck robfalck commented Apr 7, 2022

Summary

The atmosphere component included in dymos has started to be used more widely. In order to make it more accurate, it will now convert the input (geodetic) altitude to the geopotential altitude used for interpolating the atmosphere table.

In the future the default altitude type will be switched to geodetic, but for now the behavior remains the same by default.
Users can enable the geodetic input conversion by giving USatm1976Comp the h_def='geodetic' option.

Related Issues

Backwards incompatibilities

None

New Dependencies

None

…t and previous implementation) or as geodetic.
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@robfalck robfalck requested a review from Kenneth-T-Moore April 7, 2022 00:50
@coveralls
Copy link

coveralls commented Apr 7, 2022

Coverage Status

Coverage increased (+0.006%) to 95.682% when pulling 9fcf713 on robfalck:i725 into 705e4cb on OpenMDAO:master.

self._dh_dz = lambda z: (R0 / (R0 + z)) ** 2
else:
self._h = lambda z: z
self._dh_dz = lambda z: np.ones_like(z)
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it would be more efficient to just have if-branching in the compute and compute-partials functions, rather than multiplying the derivatives by an array of ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe just having it be lambda z: 1.0 would be the best compromise? Little code but a cheap operation?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that would be better than allocating an array of ones each time.

@@ -1341,11 +1357,12 @@ def compute_partials(self, inputs, partials):
d2rho_dh2 = coeffs[:, 1] + dx * (2.0 * coeffs[:, 2] + 3.0 * coeffs[:, 3] * dx)

partials['temp', 'h'] = dT_dh.ravel()
Copy link
Member

Choose a reason for hiding this comment

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

partials['temp', 'h'] is in here twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once for the assignment, once in use by partials['sos', 'h'] and once when using the chain rule if _geodetic is True. Those are all necessary I think.

@robfalck robfalck merged commit 4e89d6f into OpenMDAO:master Apr 7, 2022
@robfalck robfalck deleted the i725 branch August 25, 2022 16:09
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.

Add option for geodetic/geopotential altitude input to the atmosphere model.
3 participants