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

simple proposed fix for #289 #291

Merged
merged 1 commit into from
Aug 2, 2022
Merged

Conversation

pochedls
Copy link
Collaborator

@pochedls pochedls commented Jul 29, 2022

Description

This PR represents a simple proposed fix for #289. We just need to use .load() when handling time_lengths (used to make temporal weights. We had to do this for spatial averaging, too (e.g., here).

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

If applicable:

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass with my changes (locally and CI/CD build)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022

Codecov Report

Merging #291 (a164325) into main (16b3b06) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #291   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines         1143      1146    +3     
=========================================
+ Hits          1143      1146    +3     
Impacted Files Coverage Δ
xcdat/temporal.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16b3b06...a164325. Read the comment docs.

Copy link
Collaborator

@tomvothecoder tomvothecoder left a comment

Choose a reason for hiding this comment

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

One comment then it is good to go, thanks!

xcdat/temporal.py Outdated Show resolved Hide resolved
@pochedls
Copy link
Collaborator Author

pochedls commented Aug 2, 2022

@tomvothecoder - I think the code is fine, but I somehow re-based this in a way that makes it look like Paul's changes are on this PR (I was getting git push errors, but later realized my commits were actually reflected on origin)...

author Stephen Po-Chedley <pochedley@gmail.com> 1659104117 -0700
committer Tom Vo <tomvothecoder@gmail.com> 1659457104 -0700

simple proposed fix for #289

add suggested conditional for .load()

Co-authored-by: Tom Vo <tomvothecoder@gmail.com>

incorporate review suggestions

incorporate review suggestions

remove potentially unneeeded test
@tomvothecoder tomvothecoder force-pushed the bugfix/289-TypErrorTemporalAveraging branch from a164325 to d5642f6 Compare August 2, 2022 16:20
@tomvothecoder
Copy link
Collaborator

@tomvothecoder - I think the code is fine, but I somehow re-based this in a way that makes it look like Paul's changes are on this PR (I was getting git push errors, but later realized my commits were actually reflected on origin)...

It could have been a merge pull that placed Paul's changes on top of your's, although I'm not exactly sure why a diff still shows. I squashed your branch and rebased on the latest main, so this doesn't happen anymore.

You can merge this PR now.

@pochedls pochedls merged commit 2169b07 into main Aug 2, 2022
@pochedls pochedls deleted the bugfix/289-TypErrorTemporalAveraging branch August 2, 2022 16:32
@tomvothecoder tomvothecoder added type: bug Inconsistencies or issues which will cause an issue or problem for users or implementors. Priority: High labels Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Inconsistencies or issues which will cause an issue or problem for users or implementors.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug]: TypeError for temporal averaging with multifile datasets
3 participants