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

Adding AverageShape to daops #114

Merged
merged 7 commits into from
Feb 19, 2024
Merged

Conversation

charlesgauthier-udm
Copy link
Contributor

Pull Request Checklist:

  • [~] This PR addresses an already opened issue (for bug fixes / features)
    • This PR is related to this issue and is the continuation of this PR
  • Tests for the changes have been added (for bug fixes / features)
  • Documentation has been added / updated (for bug fixes / features)
  • HISTORY.rst has been updated (with summary of main changes)
  • I have added my relevant user information to AUTHORS.md
  • What kind of change does this PR introduce?:
    This PR adds the Spatial Averager operator to daops allowing to perform an average over a specified region

  • Does this PR introduce a breaking change?:
    Not to my knowledge

  • Other information:
    As it is, the shape parameter required by AverageShape is similar to the freq parameter of AverageTime in that it is only defined using the inputed argument. For the averqage across dimensions (Average), the dims parameter is defined as a dimension_parameter coming from roocs_utils. I was not sure if it was needed to add a shape_parameter to roocs_utils so I left it as is. If you feel like that would be preferable , please mention it in the PR and I'll add a shape_parameter to roocs_utils.

@cehbrecht
Copy link
Collaborator

  • For the averqage across dimensions (Average), the dims parameter is defined as a dimension_parameter coming from roocs_utils. I was not sure if it was needed to add a shape_parameter to roocs_utils so I left it as is. If you feel like that would be preferable , please mention it in the PR and I'll add a shape_parameter to roocs_utils

@charlesgauthier-udm If the dimension_parameter works I would keep it. We want to move roocs_utils into clisops. We could refactor roocs_utils when it has moved.

@cehbrecht
Copy link
Collaborator

@charlesgauthier-udm tests are failing due to a new intake version. Do you like to add a restriction for intake within this PR?
https://github.com/roocs/rook/blob/b5c53321179c3fdfc97ae115798f09658daacbab/environment.yml#L32

... otherwise I will open a new PR to fix this.

@charlesgauthier-udm
Copy link
Contributor Author

charlesgauthier-udm commented Feb 16, 2024

@charlesgauthier-udm tests are failing due to a new intake version.

Since the failing tests are not related to the polygon subsetting, I would suggest opening another PR, but if you don't have the time I can limit intake to 0.7.0 in this PR which seems to be working.

@cehbrecht
Copy link
Collaborator

@charlesgauthier-udm tests are failing due to a new intake version.

Since the failing tests are not related to the polygon subsetting, I would suggest opening another PR, but if you don't have the time I can limit intake to 0.7.0 in this PR which seems to be working.

@charlesgauthier-udm I will open a new PR for the intake issue :)

@cehbrecht
Copy link
Collaborator

@charlesgauthier-udm tests are working again on master (PR #115)

@cehbrecht cehbrecht self-requested a review February 19, 2024 14:15
@cehbrecht
Copy link
Collaborator

@charlesgauthier-udm I can merge if this PR is ok for you.

@charlesgauthier-udm
Copy link
Contributor Author

Yes, all good thank you for your review.

@cehbrecht cehbrecht merged commit ac85cb8 into roocs:master Feb 19, 2024
4 checks passed
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.

None yet

2 participants