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

Allow for non-pyleo rules in resample #355

Merged

Conversation

MarcoGorelli
Copy link

As requested on Slack

to be specific: I’d like resample() to accept both the usual frequency string semantics AND the ones defined in Pyleoclim (MATCH_A, MATCH_KA, MATCH_MA and MATCH_GA). It seems that it should be possible since there is no conflict in terminology between the two systems, AFAICT, but I don’t know what would need to happen under the hood.

Copy link
Collaborator

@CommonClimate CommonClimate left a comment

Choose a reason for hiding this comment

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

Thanks for doing this so quickly @MarcoGorelli ! I see that test_resample_non_pyleo_unit tests the 'MS' rule, but no other. Are you confident it would work for the other frequency strings? To be fair, it's hard to imagine anyone doing resampling at submonthly resolutions, but some coral records have biweekly resolution, so I'd want to be sure that use case is covered.

@MarcoGorelli
Copy link
Author

No worries - anything else should just be handed off to pandas, so if it works in pandas, it should work here

But I can add a test for that, sure

@CommonClimate
Copy link
Collaborator

Also, I'd like to add the resample operation to the log.
This would involve:

  • adding keep_log= False as an argument (positional?)
  • adding something like:
if keep_log == True:
    ser.log += ({len(self.log): 'resample','rule': rule},)
at the end. 

I can take care of it, but I am still fuzzy on this argument position business. Would the function call be resample(self, rule, keep_log= False, **kwargs) or resample(self, rule, **kwargs, keep_log= False) ?

@MarcoGorelli
Copy link
Author

Would the function call be resample(self, rule, keep_log= False, **kwargs) or resample(self, rule, **kwargs, keep_log= False) ?

I'd expect both to work, but the first one looks like a more common way of writing it

@CommonClimate
Copy link
Collaborator

Great. I've added the logging capability and a test for it. Merging this now

@CommonClimate CommonClimate merged commit ae4d093 into LinkedEarth:Development Mar 3, 2023
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.

2 participants