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 https://github.com/openclimatefix/ocf-ml-metrics/pull/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

• [x] Yes

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

• [ ] Yes

Checklist:

• [x] My code follows OCF's coding style guidelines
• [x] 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
• [x] I have checked my code and corrected any misspellings

[codecov[bot]]

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 :warning:

Additional details and impacted files

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[peterdudfield]

@dfulu this looks good to go?

[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: https://github.com/openclimatefix/ocf-ml-metrics/pull/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