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

Replace AbstractFloat with Real #194

Merged
merged 1 commit into from
Feb 10, 2024
Merged

Conversation

charleskawczynski
Copy link
Member

This should ensure that we can pass/promote duals through Thermodynamics

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8806d64) 93.03% compared to head (7948e59) 93.03%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #194   +/-   ##
=======================================
  Coverage   93.03%   93.03%           
=======================================
  Files          10       10           
  Lines        1149     1149           
=======================================
  Hits         1069     1069           
  Misses         80       80           

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

@charleskawczynski charleskawczynski added the Launch Buildkite Add label to launch Buildkite label Feb 9, 2024
@charleskawczynski charleskawczynski added this pull request to the merge queue Feb 10, 2024
Merged via the queue into main with commit fa610f8 Feb 10, 2024
20 checks passed
@@ -3,7 +3,7 @@ module CreateParametersExt
import Thermodynamics.Parameters.ThermodynamicsParameters
import CLIMAParameters as CP

ThermodynamicsParameters(::Type{FT}) where {FT <: AbstractFloat} =
ThermodynamicsParameters(::Type{FT}) where {FT <: Real} =
Copy link
Member

Choose a reason for hiding this comment

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

Why not remove the restriction altogether? You can test that parameter structs are built with the right type in ClimaAtmos, it may not help much to enforce this here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t think it makes sense to have a parameter struct with Ints.

Copy link
Member

@glwagner glwagner Feb 13, 2024

Choose a reason for hiding this comment

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

For sure only some types make sense, it's just you save yourself having to predict them in advance which might save you from having to write PRs like this (or eliminate the slowdown that happens when code doesn't work because of a wrong assumption like this, requiring source code update). Overrestriction of types is typical, so just trying to spread the good word...

@charleskawczynski charleskawczynski deleted the ck/rm_abstract_float branch June 10, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Launch Buildkite Add label to launch Buildkite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants