Calculate step length properly

[deinal]

Describe your changes

Updated step_length to account for dt >= 24h by calculating total seconds between steps.

Before the change a 24h step would yield a length of 0, for example.

Type of change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
• 📖 Documentation (Addition or improvements to documentation)

Checklist before requesting a review

• My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
• I have performed a self-review of my code
• For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
• I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
• I have updated the README to cover introduced code changes
• I have added tests that prove my fix is effective or that my feature works
• I have given the PR a name that clearly describes the change, written in imperative form (context).
• I have requested a reviewer and an assignee (assignee is responsible for merging). This applies only if you have write access to the repo, otherwise feel free to tag a maintainer to add a reviewer and assignee.

Checklist for reviewers

Each PR comes with its own improvements and flaws. The reviewer should check the following:

• the code is readable
• the code is well tested
• the code is documented (including return types and parameters)
• the code is easy to maintain

Author checklist after completed review

• I have added a line to the CHANGELOG describing this change, in a section
  reflecting type of change (add section where missing):
  • added: when you have added new functionality
  • changed: when default behaviour of the code has been changed
  • fixes: when your contribution fixes a bug

Checklist for assignee

• PR is up to date with the base branch
• the tests pass
• author has added an entry to the changelog (and designated the change as added, changed or fixed)
• Once the PR is ready to be merged, squash commits and merge the PR.

[deinal]

@joeloskarsson fixed a minor issue with step length calculation >= 24h. Reviewer and assignee needed here

[joeloskarsson]

This change looks good to me, but there are issues with the tests that are unrelated to this PR. We will have to fix those first, before we can merge this. I'll just approve this for now, and we can merge it once that is fixed.

I think the linting failing is just because we are using an old version of https://github.com/pre-commit/action, that stopped working when gh cache servers were updated. The pip test requires a bit more digging, but sadly I don't have capacity for solving it right now, so it will have to wait a bit.

[observingClouds]

Hi @deinal thanks for the PR. I think it would be great if we could generalize this more to any dt and not just multiple of hours. We might not need milliseconds, but having dt in seconds would be a great feature. @deinal would you feel comfortable to do such a change and also have the time for it?

[observingClouds]

#140 potentially fixes the testing issues unrelated to this PR

[joeloskarsson]

Oh that is great with the tests 😄 I clearly have not kept up with things here for a bit.

I could see a value in not restricting the step length to hours. How do you envision that being implemented @observingClouds ? As separate functions for seconds/minutes/hours, or as some argument you give to step_length? As it just returns an int we could not just return different things depending on the temporal resolution of the data, as that would become ambiguous. I guess the better solution would be to return a timedelta. However reworking the function and its downstream usage is maybe a bit much for just a bugfix.

[observingClouds]

Yeah, I envision the latter, where we return a timedelta object or the steps in seconds (instead of hours). If this was meant to just be a bugfix my suggestion should not stop this.

[deinal]

Hi @observingClouds generalizing dt seems like a good idea! However, I'd leave this pr as just a bugfix. Perhaps a new enhancement issue could be opened for this.

[observingClouds]

@deinal the CICD should be fixed now. I am updating your branch to see if the tests will also succeed for your new contribution :)

[joeloskarsson]

Seems that the test actually caught something, so that is useful 😃 (I did not even know we had a test for step length). Function says it should return int, so maybe reasonable to just cast the return value to int.