CliMA / Thermodynamics.jl

A package containing a library of moist thermodynamic relations.
https://clima.github.io/Thermodynamics.jl/dev/
Apache License 2.0
61 stars 2 forks source link

Introduce logger type, remove module methods #158

Open charleskawczynski opened 1 year ago

charleskawczynski commented 1 year ago

This PR removes print_warning and error_on_non_convergence, which allow users to toggle how Thermodynamics does error handling. With this PR, those methods are removed, and instead users pass in objects that dispatch this error handling. One advantage of this is that dispatch will result in statically elided paths. The upsides of this is that: 1) JET won't complain (although we can currently just overload print_warning and error_on_non_convergence to silence JET) 2) this PR could result in less register usage

I've looked at NVTX reports for several jobs, and the constructors (where this feature is relevant) are not really a bottleneck. So, 2) may not actually buy us anything.

codecov[bot] commented 1 year ago

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (20de40f) 92.96% compared to head (3d02b13) 92.43%. Report is 7 commits behind head on main.

Files Patch % Lines
src/relations.jl 89.32% 11 Missing :warning:
src/logger.jl 83.33% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #158 +/- ## ========================================== - Coverage 92.96% 92.43% -0.53% ========================================== Files 10 11 +1 Lines 1152 1164 +12 ========================================== + Hits 1071 1076 +5 - Misses 81 88 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

trontrytel commented 1 year ago

Is that something I could also use in CloudMicrophysics to print warnings when someone is trying to use the parameterizations outside of the acceptable range of inputs?

I don't want to throw an error. But it would be great to have a warning of some sort

charleskawczynski commented 1 year ago

Is that something I could also use in CloudMicrophysics to print warnings when someone is trying to use the parameterizations outside of the acceptable range of inputs?

Yes

I don't want to throw an error. But it would be great to have a warning of some sort

Generally, this can be used as follows:

if logger isa WarnAndErrorLogger
 # do stuff that is not gpu compatible
end

That way, users can call different style versions:

I'm not sure I like the name "logger" though, it's not solely for logging. It's kind of like a log level, maybe that's a better name?

simonbyrne commented 1 year ago

@vchuravy this is where I wanted some sort of GPU logger.

I'm not 100% sure of names. I would prefer if it didn't get conflated with the existing Julia logging though.

charleskawczynski commented 1 year ago

It seems like we may no longer have a choice in CUDA 5.0: https://buildkite.com/clima/climaatmos-ci/builds/13625#018b0189-731b-4c9d-970f-554d7c5740b1/145-235. Going to move forward with this. We don't need to actually use this, but it will nevertheless help with upstream inference.

charleskawczynski commented 1 year ago

@simonbyrne, it seems that this functionality will help us get https://github.com/CliMA/ClimaAtmos.jl/pull/2199 merged in. Unless there are naming objections/suggestions, I'd like to merge this.

Sbozzolo commented 7 months ago

Can you please give a brief high-level overview on how this works?

charleskawczynski commented 7 months ago

Yes. Although, I keep having back-and-forth thoughts on this.

One downside that I'm remembering is that: The src code (e.g., in ClimaAtmos) will need to be changed if we leverage this feature to silence JET noise. Right now, we can just use TD. print_warning() = false at the top of the JET script.

Maybe we can put this on hold.