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

Add KGE Score #700

Merged
merged 12 commits into from
Oct 7, 2024
Merged

Add KGE Score #700

merged 12 commits into from
Oct 7, 2024

Conversation

tennlee
Copy link
Collaborator

@tennlee tennlee commented Sep 19, 2024

This incorporates the work done by @durgals to add the KGE score (Issue #678, PR #679) and additionally includes some changes I applied in response to the code review on PR #679.

@durgals the test coverage is not 100%. I have included a screenshot showing which lines do not have test coverage. I have also pushed this work to the 228 documentation branch (which we use to test the documentation) Please go through the checklist below and check each thing off.

image

Also, if you are unhappy with any of the changes I made, please feel free to say how it should be changed.

durgals and others added 2 commits September 19, 2024 19:46
* KGE scores
Added KGE notebook to tutorial gallery, and adjusted capitalisation
---------

Signed-off-by: Tennessee Leeuwenburg <134973832+tennlee@users.noreply.github.com>
Co-authored-by: shr015 <Durgalal.Shrestha@csiro.au>
Co-authored-by: Tennessee Leeuwenburg <tennessee.leeuwenburg@bom.gov.au>
Co-authored-by: Tennessee Leeuwenburg <134973832+tennlee@users.noreply.github.com>
Co-authored-by: Stephanie Chong <168800785+Steph-Chong@users.noreply.github.com>
Co-authored-by: Nicholas Loveday <48701367+nicholasloveday@users.noreply.github.com>
src/scores/continuous/standard_impl.py Outdated Show resolved Hide resolved
src/scores/continuous/standard_impl.py Outdated Show resolved Hide resolved
@durgals

This comment was marked as resolved.

@tennlee

This comment was marked as resolved.

@durgals

This comment was marked as resolved.

@tennlee

This comment was marked as resolved.

@durgals

This comment was marked as resolved.

@durgals

This comment was marked as resolved.

@tennlee

This comment was marked as resolved.

tennlee and others added 2 commits September 20, 2024 16:27
Co-authored-by: Stephanie Chong <168800785+Steph-Chong@users.noreply.github.com>
Signed-off-by: Tennessee Leeuwenburg <134973832+tennlee@users.noreply.github.com>
Co-authored-by: Stephanie Chong <168800785+Steph-Chong@users.noreply.github.com>
Signed-off-by: Tennessee Leeuwenburg <134973832+tennlee@users.noreply.github.com>
@tennlee

This comment was marked as resolved.

durgals pushed a commit to durgals/scores that referenced this pull request Sep 20, 2024
Signed-off-by: Tennessee Leeuwenburg <134973832+tennlee@users.noreply.github.com>
Co-authored-by: shr015 <Durgalal.Shrestha@csiro.au>
Co-authored-by: Tennessee Leeuwenburg <134973832+tennlee@users.noreply.github.com>
@tennlee
Copy link
Collaborator Author

tennlee commented Sep 22, 2024

@durgals @nicholasloveday

Thanks for that PR Durga. I have merged it into this PR. So far as I know, everything is now ready to be merged. I have pushed the updates to https://scores.readthedocs.io/en/228-documentation-testing-branch/ .

Could you both please take a final look and let me know with a short message here? If you're both happy I will go ahead and merge this into develop.

Note - rather than squashing this into one single commit, I will probably organise it into two commits, but I will take care of that as part of the merge process - neither of you need to do anything about it.

tennlee and others added 3 commits September 22, 2024 13:54
Co-authored-by: Stephanie Chong <168800785+Steph-Chong@users.noreply.github.com>
Signed-off-by: Tennessee Leeuwenburg <134973832+tennlee@users.noreply.github.com>
@nicholasloveday
Copy link
Collaborator

I think that this is fine to merge in. Thank you for this work.

A couple of minor things to mention.

  • Consider removing the dots in the KGE equation (in case someone thinks that this is the dot product). I am still okay if this is merged in as is.
  • In the future, I think that we should try to provide more support for supporting xr.Datasets as inputs. No changes needed to this at this stage.
  • Did you want "are the scaling factors for the correlation coefficient" bold in the docstring?

@tennlee tennlee merged commit 05bc6f9 into develop Oct 7, 2024
24 checks passed
@tennlee
Copy link
Collaborator Author

tennlee commented Oct 7, 2024

I have merged this into the develop branch now. @durgals can you please review @nicholasloveday 's final comments and consider if you would like to raise new issues to track them. I'm fine either way in each case.

@durgals
Copy link
Contributor

durgals commented Oct 8, 2024

@tennlee Thank you merging this. Regarding comments from @nicholasloveday:

  • Consider removing the dots in the KGE equation (in case someone thinks that this is the dot product). I am still okay if this is merged in as is.
    I will update this in the next round of changes, including support for xr.Dataset or weights.
  • In the future, I think that we should try to https://github.com/nci/scores/issues/702for supporting xr.Datasets as inputs. No changes needed to this at this stage.
    I’ll consider this for a future release,
  • Did you want "are the scaling factors for the correlation coefficient" bold in the docstring?
    I already reported this to @tennlee, and he mentioned that it’s a rendering issue with ReadTheDocs.

@tennlee tennlee deleted the kge-staging branch October 28, 2024 08:11
@tennlee tennlee mentioned this pull request Nov 1, 2024
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.

4 participants