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

Changes minutes to hours on forecast horizons tables/plots #177

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Arunesh-Tiwari
Copy link

@Arunesh-Tiwari Arunesh-Tiwari commented Aug 4, 2024

Pull Request

Description

Changed minutes to hours on forecast horizons tables/plots.

Fixes #167

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration

  • Yes

If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?

  • Yes

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

Changed minutes to hours on forecast horizons tables/plots.
@Arunesh-Tiwari Arunesh-Tiwari changed the title #167 Issue: solved #167 Issue: Changes minutes to hours on forecast horizons tables/plots Aug 4, 2024
@Arunesh-Tiwari Arunesh-Tiwari changed the title #167 Issue: Changes minutes to hours on forecast horizons tables/plots Changes minutes to hours on forecast horizons tables/plots Aug 4, 2024
@peterdudfield
Copy link
Contributor

Thanks for this, would you also be able to change it here

@Arunesh-Tiwari
Copy link
Author

Apologies for the delayed response. I've made the suggested changes and would appreciate your review.

@@ -133,14 +133,28 @@ def metric_page():
make_recent_summary_stats(values=y_mae)
make_recent_summary_stats(values=y_rmse, title="Recent RMSE")

def convert_minutes_to_hours(minutes):
return round(minutes / 60, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably only define this once

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps import from src/plots/forecast_horizon

@peterdudfield
Copy link
Contributor

Could you add a screen shot to the PR? Or perhaps you didnt run this locally?

@Arunesh-Tiwari
Copy link
Author

Arunesh-Tiwari commented Aug 19, 2024

Could you add a screen shot to the PR? Or perhaps you didnt run this locally?

I didn't run this locally. I was trying to setup it locally but I guess only OCF team members can connect to the forecast development database using these Notion instructions(as mentioned in readme).

@Arunesh-Tiwari
Copy link
Author

@peterdudfield Could I kindly request access to the database to resolve this issue? If that's not feasible, would it be better to close the pull request, or is there an alternative approach that I could take?

@peterdudfield
Copy link
Contributor

Sorry, thats not feasible, you could run your own database using nowcasting_Datamodel, but the database will be empty

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.

Changes minutes to hours on forecast horizons tables/plots
2 participants