added new private method for filtering on daily aggregated data

NREL / rdtools

PV Analysis Tools in Python

https://rdtools.readthedocs.io/

MIT License

158 stars 67 forks source link

added new private method for filtering on daily aggregated data #348

Closed kperrynrel closed 1 year ago

kperrynrel commented 2 years ago

[x] Code changes are covered by tests
[x] Code changes have been evaluated for compatibility/integration with TrendAnalysis
[x] New functions added to __init__.py
[x] API.rst is up to date, along with other sphinx docs pages
[x] Example notebooks are rerun and differences in results scrutinized
[x] Updated changelog

kperrynrel commented 2 years ago

Only private methods updated, no API.rst or init.py updates needed

kperrynrel commented 2 years ago

Hey @kanderso-nrel and @mdeceglie I just added a unit test for the daily ad hoc filter and I wanted to get your input. The daily filter MUST have the same timestamp index as the aggregated data currently, which means there's a time zone attached:

Are we good with this? It means we need to make sure all daily filters return a series that is tz-aware.

kandersolar commented 2 years ago

In general (outside the context of TrendAnalysis) I think it makes sense to require our filter functions be implemented such that their outputs are tz-consistent with their inputs, so I think it's fine for TrendAnalysis to assume that if it passes a tz-aware aggregated Series to a filter function then it'll get a tz-aware mask back.

mdeceglie commented 2 years ago

@cdeline Did you mention that you still have a bug with this? Could you provide an example that occurs with the most up to date version of this branch? Thanks!

kandersolar commented 2 years ago

linter failures are from #351 and can be ignored