Fix bug with activity limits

[tsmbland]

A bug related to annual activity limits (which we have in a few of the examples). Apologies, not sure how this crept in.

This is the bug (passing TimeSliceInfo::default()), but also some refactoring changes so that time slice info doesn't need to be passed around

I had already fixed this in #1021, but easier to review it as a separate PR

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.44%. Comparing base (db2c40a) to head (8f9ba79).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1036      +/-   ##
==========================================
- Coverage   81.45%   81.44%   -0.01%     
==========================================
  Files          52       52              
  Lines        6497     6494       -3     
  Branches     6497     6494       -3     
==========================================
- Hits         5292     5289       -3     
+ Misses        949      948       -1     
- Partials      256      257       +1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[alexdewar]

Makes sense to review separately. It looks ok on the face of it, though I'm not familiar with the code.

How come the tests didn't catch it btw? Was it because they were using something like TimeSliceInfo::default() anyway so the results matched up?

[tsmbland]

How come the tests didn't catch it btw? Was it because they were using something like TimeSliceInfo::default() anyway so the results matched up?

Partly because I didn't include a unit test covering annual limits (now fixed), and possibly a little bit because of poor code design...

TimeSliceInfo::default() has one seasonal timeslice "all-year". This was getting passed to get_limit_for_season within get_limit_for_year, but get_limit_for_season doesn't actually complain if an invalid season is passed (I didn't think this could ever happen), ultimately returning a limit of [0,0]. In add_annual_limit, it only adds annual limits if more restrictive than the current limit set by seasonal constraints, but since it was seeing all seasonal limits as [0,0], the annual limits weren't being added to the model.

The regression tests did pick this up in a way because the results changed, but this was in the context of a larger PR where I was expecting the results to change anyway.

I was wondering whether TimeSliceInfo::default() should be restricted for use by tests, to avoid any chance of something like this happening again, but we do actually use it in the main code if the user doesn't provide a timeslices.csv file, so I guess not.