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

Percentile outlier QC #183

Merged
merged 3 commits into from
Nov 7, 2023
Merged

Percentile outlier QC #183

merged 3 commits into from
Nov 7, 2023

Conversation

ladsmund
Copy link
Contributor

No description provided.

* Implemented core functionality for season based threshold filter
* Implemented script to compute thresholds using historical L1 (inferred) data
* Added default thresholds
* Updated setup.py to allow thresholds data file
@ladsmund ladsmund force-pushed the features/percentile_qc branch from 6b45719 to 475805f Compare November 2, 2023 15:14
@ladsmund ladsmund requested a review from PennyHow November 2, 2023 15:18
Copy link
Member

@PennyHow PennyHow left a comment

Choose a reason for hiding this comment

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

All looks okay. The only thing is that there is an incompatibility with Python 3.8 in the process test (see screenshot below of Github Action read-out). We run on Python 3.8 on both azure-aws and glacio01, so either we need to solve this incompatibility or update everything to at least Python 3.9.

Screenshot from 2023-11-02 13-28-25

I think it is related to the syntax of this line including the type bool as a subscript of the output np.ndarray.

def get_season_index_mask(data_set: pd.DataFrame, season: str) -> np.ndarray[bool]:

I tried changing it to this:

def get_season_index_mask(data_set: pd.DataFrame, season: str) -> np.ndarray

But then I get another error (unrelated to the previous syntax incompatibility). I guess this new error is linked to another compatibility issue to Python 3.8:

Traceback (most recent call last):
  File "/home/pho/anaconda3/envs/pypromice/bin/get_l3", line 8, in <module>
    sys.exit(get_l3())
  File "/home/pho/anaconda3/envs/pypromice/lib/python3.8/site-packages/pypromice/process/get_l3.py", line 39, in get_l3
    aws.process() 
  File "/home/pho/anaconda3/envs/pypromice/lib/python3.8/site-packages/pypromice/process/aws.py", line 78, in process
    self.getL2()
  File "/home/pho/anaconda3/envs/pypromice/lib/python3.8/site-packages/pypromice/process/aws.py", line 99, in getL2
    self.L2 = toL2(self.L1A, vars_df=self.vars)
  File "/home/pho/anaconda3/envs/pypromice/lib/python3.8/site-packages/pypromice/process/L1toL2.py", line 73, in toL2
    ds = outlier_detector.filter_data(ds)                                 # Flag and remove percentile outliers
  File "/home/pho/anaconda3/envs/pypromice/lib/python3.8/site-packages/pypromice/qc/percentiles/outlier_detector.py", line 70, in filter_data
    if not stid_thresholds.any():
  File "/home/pho/anaconda3/envs/pypromice/lib/python3.8/site-packages/pandas/core/generic.py", line 1526, in __nonzero__
    raise ValueError(
ValueError: The truth value of a Series is ambiguous. Use a.empty, a.bool(), a.item(), a.any() or a.all().

@ladsmund, have you tried running this PR on Python 3.8 to check it runs? I'm just cloning your branch, installing it into my Python environment and then running the get_l3 function on a random station.

src/pypromice/qc/percentile.py Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
src/pypromice/qc/percentile.py Outdated Show resolved Hide resolved
src/pypromice/qc/percentile.py Outdated Show resolved Hide resolved
@ladsmund ladsmund requested a review from PennyHow November 6, 2023 14:27
@PennyHow
Copy link
Member

PennyHow commented Nov 6, 2023

All runs fine now @ladsmund! Feel free to merge.

@ladsmund ladsmund marked this pull request as ready for review November 7, 2023 08:32
@ladsmund ladsmund merged commit 96a098d into main Nov 7, 2023
5 checks passed
@PennyHow PennyHow mentioned this pull request Nov 7, 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.

3 participants