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

make baseload advice longterm table use academic years #4124

Merged
merged 23 commits into from
Jan 10, 2025

Conversation

tbhi
Copy link
Contributor

@tbhi tbhi commented Dec 19, 2024

No description provided.

@tbhi tbhi marked this pull request as ready for review January 7, 2025 10:01
Copy link
Collaborator

@ldodds ldodds left a comment

Choose a reason for hiding this comment

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

Changes to the calculations look correct and have briefly tested locally so I think this is OK to merge.

However the original plan was for the academic year changes for the long term, baseload and tariffs pages to be behind the same feature flag so they can be enabled/disabled together. Looks like it'll be fiddly to refactor to that now, so suggest we just focus on getting this released and ensuring its been manually tested on test server before prod.

So merge this and other PRs tomorrow for release Monday

I'd like to later refactor the baseload service to move the calculations you've added into a service in the analytics so we're retaining a standard API for the basic analysis. This will later allow us to move to generating more of these metrics overnight. The services/school/advice/* classes are already fairly complicated.

It doesn't look like the BaseloadAnnualBreakdownService is actually being used so that could be refactored to look something like this:

BaseloadAnnualBreakdownService
 def initialize(meter) # real or aggregate meter

 # returns hash of 
 # period: (a SchoolPeriod), 
 # partial:, 
 # baseload_kw:, 
 # baseload_usage: (a CombinedUsageMetric)
 # 
 # can be extended to support :year, :academic_year
 def annual_baseload_breakdown(period: :fixed_academic_year)

 # as per current code
 def enough_data?
 def data_available_from
 
end 

Base automatically changed from 4619-academic-year-comparisons-on-long-term-usage-pages to master January 10, 2025 12:19
@tbhi tbhi merged commit c1db296 into master Jan 10, 2025
21 checks passed
@tbhi tbhi deleted the 4691-academic-year-comparisons-for-baseload branch January 10, 2025 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants