-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
61b63f4
to
1c4bb7d
Compare
There was a problem hiding this 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)), | ||
] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, what for though?
There was a problem hiding this comment.
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)
Also, this will break this in ClimaAtmos: |
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. |
1c4bb7d
to
0ab9f34
Compare
Codecov ReportAttention: Patch coverage is
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. |
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.