Fixed error caused by incorrect call to common_metrics in validation step

[confusedmatrix]

Pull Request

Description

This PR fixes a typo in a parameter name when calling common_metrics in the validation step during training of PVNet. Additionally, as common_metrics expects np.ndarrays as input, the input predictions and target values are cast from torch Tensors to np.ndarrays.

This PR works in conjunction with openclimatefix/ocf-ml-metrics#13 to provide correct logging of validation metrics during training using multiple forecast horizons.

How Has This Been Tested?

Tested by training locally over pre-made data batches

• 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

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (d0a80ee) 50.85% compared to head (b341182) 50.85%.
Report is 8 commits behind head on main.

Files	Patch %	Lines
pvnet/models/base_model.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #105   +/-   ##
=======================================
  Coverage   50.85%   50.85%           
=======================================
  Files          26       26           
  Lines        1701     1701           
=======================================
  Hits          865      865           
  Misses        836      836           

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

[peterdudfield]

@dfulu this looks good to go?

[dfulu]

If I'm correct I think that common_metrics returns scalar values for the rmse and mae. mse_each_step is expected to be an array of mse values for each forecast step. Same with mae_each_step. So I think there is still some work to do to fix this

[peterdudfield]

If I'm correct I think that common_metrics returns scalar values for the rmse and mae. mse_each_step is expected to be an array of mse values for each forecast step. Same with mae_each_step. So I think there is still some work to do to fix this

but this is about the inputs?
Also perhaps a test for this would be a good way to check this?

[confusedmatrix]

If I'm correct I think that common_metrics returns scalar values for the rmse and mae. mse_each_step is expected to be an array of mse values for each forecast step. Same with mae_each_step. So I think there is still some work to do to fix this

but this is about the inputs? Also perhaps a test for this would be a good way to check this?

Yep - the PR in ocf-ml-metrics (here: openclimatefix/ocf-ml-metrics#13) updates the common_metrics function to handle scalar and array return types + includes tests for both cases

[dfulu]

Thanks @confusedmatrix, I merged your changes to ocf-ml-metrics, so once the build and pip release is done we should update the requirements here and that should be it.

Also @peterdudfield we probably should have a full end2end test for the PVNet training, loop, but I'd say lets do that in a separate issue

[peterdudfield]

Thanks @confusedmatrix, I merged your changes to ocf-ml-metrics, so once the build and pip release is done we should update the requirements here and that should be it.

Also @peterdudfield we probably should have a full end2end test for the PVNet training, loop, but I'd say lets do that in a separate issue

I agree, year put that in a different issue

[dfulu]

ocf-ml-metrics 0.0.11 has been release so I think this is good to go now