LinkedEarth / Pyleoclim_util

Python Package for the Analysis of Paleoclimate Data. Documentation at
https://pyleoclim-util.readthedocs.io
GNU General Public License v3.0
88 stars 33 forks source link

Update series.py #485

Closed neillinehan closed 10 months ago

neillinehan commented 10 months ago

Fixes https://github.com/LinkedEarth/Pyleoclim_util/issues/484#issue-2053405788

ntau not in wwz_coherence() arguments, so pop it out of the settings dict after calculations are complete.

CommonClimate commented 10 months ago

Hi @neillinehan thanks for suggesting this fix. Did you end up creating a pytest for it as well? This is something we encourage for any change to the code, and you already have all it takes to make one. Another thing is that you need to nominate someone to review your code. I think @fzhu2e would be best if he has time, as this functionality is his baby, and he knows that part of the code the best. Out of curiosity, what are you trying to do with the code?

neillinehan commented 10 months ago

Hi @CommonClimate,

The proposed fix works in my local Jupyter notebook environment. I’ve tested the changes informally and everything works as expected. I'm new to GitHub and finding the process of writing formal tests a bit time-consuming at the moment, I understand if this means my current contribution cannot be accepted. I may return to this at a later time.

I'm exploring seasonal changes in some crop light responses sampled at a fairly high frequency (1-min), so I need higher time-resolution as opposed to the default high spectral-resolution.

CommonClimate commented 10 months ago

Hi @neillinehan , I understand unit tests may not be your bread and butter, but as it stands I have no easy way to test the veracity of your claim ("The proposed fix works in my local Jupyter notebook environment"), or more importantly that it works in an arbitrary environment. Would you be able to repurpose the docstring example using AIR and NINO3 and making sure that your fix works for that? Please share your code, if so, and I'll turn that into a unit test prior to merging. Thank you in advance for easing our process! Best, Julien

CommonClimate commented 10 months ago

addresses issue #484