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

Add profile abstraction, improve tests #45

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Jul 3, 2024

This PR adds an abstraction to make testing easier. I think that this is technically non-breaking, since all of the returned objects are still callable. One nice thing about this is that now the return type (at least for dispatching), is much more narrow.

This PR also fixes up the tests and makes what is tested much more explicit. In doing this, I found and fixed a few more type instabilities.

Maybe we can merge this and then rebase #44?

I think doing merging this first may help debugging what is wrong in #44.

Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

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

I am not completely convienced we need new types, but I also don't think that users would be able to tell the difference from regular functions (given that they are callable), so this does not hurt and encodes a bit of extra information.

test/profiles.jl Outdated
(:TRMM_LBA_tke_prescribed, APL.TRMM_LBA_tke_prescribed(FT)),
(:TRMM_LBA_u, APL.TRMM_LBA_u(FT)),
(:TRMM_LBA_v, APL.TRMM_LBA_v(FT)),
]
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add a new line 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.

Sure, what for though?

Copy link
Member

Choose a reason for hiding this comment

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

So that we end all files with a newline character, otherwise they mess up the terminal when you print them (because the prompt stays on the same line)

@Sbozzolo
Copy link
Member

Sbozzolo commented Jul 3, 2024

@charleskawczynski
Copy link
Member Author

charleskawczynski commented Jul 3, 2024

Also, this will break this in ClimaAtmos: https://github.com/CliMA/ClimaAtmos.jl/blob/d36c0f647c351b7fc066545a998b892bf38c141d/src/initial_conditions/initial_conditions.jl#L713

Ah, right, that's unfortunate.

I guess that'll even break with #44. This will at least allow us to generalize that code a bit more easily. I'll go ahead and merge.

Copy link

codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 97.58065% with 3 lines in your changes missing coverage. Please review.

Project coverage is 80.50%. Comparing base (5abf8a6) to head (0ab9f34).
Report is 4 commits behind head on main.

Files Patch % Lines
src/profiles/ARM_SGP.jl 82.35% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              main      #45       +/-   ##
============================================
- Coverage   100.00%   80.50%   -19.50%     
============================================
  Files           14       15        +1     
  Lines          271      436      +165     
============================================
+ Hits           271      351       +80     
- Misses           0       85       +85     

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

@charleskawczynski charleskawczynski merged commit 6aa54b3 into main Jul 3, 2024
6 of 8 checks passed
@charleskawczynski charleskawczynski mentioned this pull request Jul 3, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants