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 roll function for patches #382

Merged
merged 5 commits into from
Jun 4, 2024
Merged

Adding roll function for patches #382

merged 5 commits into from
Jun 4, 2024

Conversation

Nik-P2
Copy link
Contributor

@Nik-P2 Nik-P2 commented May 31, 2024

Description

Adding in roll function for patches
based on numpy roll function
an example of this function

import dascore as dc
patch = dc.get_example_patch()
# roll time dimension 5 elements
rolled_patch = patch.roll(time=5, samples=True)

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes.
  • documented the new feature with docstrings or appropriate doc page.
  • included a test. See testing guidelines.
  • your name has been added to the contributors page (docs/contributors.md).
  • added the "ready_for_review" tag once the PR is ready to be reviewed.

@Nik-P2 Nik-P2 added ready_for_review PR is ready for review patch related to Patch class labels May 31, 2024
Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.54%. Comparing base (c2f6ca7) to head (82e143c).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #382   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files          97       97           
  Lines        7633     7647   +14     
=======================================
+ Hits         7598     7612   +14     
  Misses         35       35           
Flag Coverage Δ
unittests 99.54% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

@d-chambers d-chambers left a comment

Choose a reason for hiding this comment

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

Looks great!


roll_arr = np.roll(arr, value, axis=axis)

return patch.new(data=roll_arr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to update the coord as well? Or maybe just adding an optional update_coord argument so when is set True, the function will roll the coord as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to update the coord as well? Or maybe just adding an optional update_coord argument so when is set True, the function will roll the coord as well

Ya, good point. Normally Patch.roll would be used to create a new patch that will broadcast with the old one. For example, in cross correlation. In this case you wouldn't want to coords to update, you are purposefully "rolling" the data around stationary coordinates. However, I could also see a case where you might want to roll the coords as well, so an optional argument to do so makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Nik-P2, do you think you can implement this?

You need to add an optional keyword called update_coord or roll_coord which is False by default. Then if it is True do something like this (haven't tested but this should be enough to get you started):

...
if roll_coord:  # Handle rolling coordinate if needed
    coord = patch.get_coord(dim)
    rolled_coord_array = np.roll(coord.values, value, axis)
    new_coord = coord.update(values=rolled_coord_array)
    patch = patch.update_coords(**{dim: new_coord})

return patch.new(data=roll_arr)

Make sure to write another test case that runs this logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I will work on that now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks great. Thanks, @Nik-P2 !

@d-chambers
Copy link
Contributor

Hey @Nik-P2,
If you look at the CI that failed (by clicking on the details button above) I see this error message when running the docstests. I think I told you wrong above, the dimensional coordinates are always 1D, so you might try replacing this line:

np.roll(coord.values, value, axis=axis)

with this one:

np.roll(coord.values, value, axis=0)

and see if it works.

@d-chambers
Copy link
Contributor

All green, feel free to press the squash and merge button.

@Nik-P2 Nik-P2 merged commit 654ee0f into master Jun 4, 2024
15 checks passed
@d-chambers d-chambers deleted the patch_roll branch June 4, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch related to Patch class ready_for_review PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants