updates for Insolation.jl

[claresinger]

• remove lon_perihelion parameter
• update epoch to be given as a String

@charleskawczynski Do you want to wait until you merge the changes to the code structure? Or can we merge this sooner?

[charleskawczynski]

Does any other package use lon_perihelion? Deleting it is a breaking change, and upgrading to breaking changes of CLIMAParameters is a lot of work. Can we just not delete it for now?

[claresinger]

Does any other package use lon_perihelion? Deleting it is a breaking change, and upgrading to breaking changes of CLIMAParameters is a lot of work. Can we just not delete it for now?

I can't imagine any other package uses it. But if it's easier to have unused parameters floating around then we can just keep it.

[claresinger]

@odunbar I think we need to update the tests? The line k_value ≈ cp_k(param_set_cpp) doesn't work for comparing strings, right?

Updating to this makes tests pass, but I'm sure there's something more elegant to do.

if typeof(k_value) == String
    @test cmp(k_value, cp_k(param_set_cpp)) == 0
else
    @test (k_value ≈ cp_k(param_set_cpp))
end

[trontrytel]

What is the status on this one? @claresinger - should we update the new toml files with those parameters?

I think that it's better to delete unused parameters, even if it means a breaking change. Maybe we could collect a couple of such changes and then do it together?

[claresinger]

Hi @trontrytel. The status on this was 1) waiting for parameters to accept string values (which has happened) and 2) waiting to delete the unused lon_perihelion param because it will be a breaking change and I guess we decided it was easier to just keep it around for now. If you want to collect together a handful of breaking changes, that's great! I'll update the PR now.

[claresinger]

bors try

[bors]

try

Build succeeded:

• format
• ci 1.7.0 - macOS-latest
• ci 1.7.0 - windows-latest
• ci 1.7.0 - ubuntu-latest
• docbuild

[trontrytel]

Sounds good, thank you @claresinger! I also have one name change that would be a breaking thing to merge in. I'll collect the two together and do a breaking release. (And maybe see if other people have some breaking changes they want to make too). @nefrathenrici - should we do it before or after your aerosol PR?

[nefrathenrici]

@trontrytel Feel free to do it before my PR, I don't mind adding a minor release afterwards if needed.

[codecov]

Codecov Report

Base: 94.26% // Head: 94.26% // No change to project coverage 👍

Coverage data is based on head (11c33a1) compared to base (26bde4d).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #88   +/-   ##
=======================================
  Coverage   94.26%   94.26%           
=======================================
  Files           1        1           
  Lines         122      122           
=======================================
  Hits          115      115           
  Misses          7        7           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.