Skip to content

Conversation

@MarionBWeinzierl
Copy link
Collaborator

@MarionBWeinzierl MarionBWeinzierl commented Apr 15, 2025

Description

The underlying issue #416 wanted to add a check for SubdailyScaler to make sure that the data is indeed subdaily. The scaler has in the meantime been replaced by the AcclimationModel, which is paired with the SubdailyPModel.

Fixes #416

Type of change

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist

  • Make sure you've run the pre-commit checks: $ pre-commit run -a
  • All tests pass: $ poetry run pytest

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.92%. Comparing base (b65d912) to head (0579876).
Report is 13 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #485   +/-   ##
========================================
  Coverage    96.92%   96.92%           
========================================
  Files           38       38           
  Lines         2955     2957    +2     
========================================
+ Hits          2864     2866    +2     
  Misses          91       91           

☔ 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.

@MarionBWeinzierl MarionBWeinzierl self-assigned this Apr 15, 2025
@davidorme
Copy link
Collaborator

Or does all data in AcclimationModel need to be subdaily -- I suppose not?

The SubdailyPModel and AcclimationModel are basically paired - I don't think we should allow the AcclimationModel to create inputs that SubdailyPModel will reject, so it is probably better placed in AcclimationModel.

It's also marginally more convenient since there is already one check on the spacing attribute:

if spacing < 0:
raise ValueError("Datetime sequence must be increasing")

@MarionBWeinzierl MarionBWeinzierl force-pushed the issue416_check_subdaily branch from db23dc5 to aa6f9b6 Compare May 1, 2025 14:31
Copy link
Collaborator

@davidorme davidorme left a comment

Choose a reason for hiding this comment

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

LGTM - can we add a test?

@MarionBWeinzierl
Copy link
Collaborator Author

LGTM - can we add a test?

I added a test now

@MarionBWeinzierl MarionBWeinzierl merged commit accde33 into develop May 2, 2025
16 checks passed
@github-project-automation github-project-automation bot moved this from Review to Done in ICCS development board May 2, 2025
@MarionBWeinzierl MarionBWeinzierl deleted the issue416_check_subdaily branch May 2, 2025 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Add failure for scaler if not subdaily

3 participants