Skip to content

Performance Improvement#562

Open
leilabbb wants to merge 2 commits intomainfrom
glider_qc_copilot-_patches
Open

Performance Improvement#562
leilabbb wants to merge 2 commits intomainfrom
glider_qc_copilot-_patches

Conversation

@leilabbb
Copy link
Copy Markdown
Contributor

Verified suggestions by copilot. The main ones are:

  • added " if statements " to catch extreme cases
  • fixed function return to formulate lists

Verified suggestions by copilot. The main ones are:
- added " if statements " to catch extreme cases
- fixed function return  to formulate lists
@leilabbb
Copy link
Copy Markdown
Contributor Author

pre-commit.ci autofix

@leilabbb leilabbb requested a review from benjwadams April 13, 2026 22:27
Comment thread glider_qc/glider_qc.py

# Check if it's an array of fill values
if unique_vals == inp._FillValue:
if unique_vals.item == inp._FillValue:
Copy link
Copy Markdown
Contributor

@benjwadams benjwadams Apr 17, 2026

Choose a reason for hiding this comment

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

The removed version will never be true since numpy.unique returns a 1D array if the axis argument is left unspecified or None. https://numpy.org/doc/stable/reference/generated/numpy.unique.html

The second added version (which admittedly Copilot suggested) will fail and throw an exception for anything but a single-element array, which is not going to be the majority of datasets/variables. I would probably just compare unique_vals == np.array([inp._FillValue]) or something similar, but I don't know how this would operate for masked arrays.

>>> import numpy as np
>>>arr = np.array([1, 2])
>>> arr.item()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: can only convert an array of size 1 to a Python scalar

Please address and add unit tests for this case.

@benjwadams
Copy link
Copy Markdown
Contributor

Commit message should be more specific. Right now I wouldn't know what code the changes affected looking at the commit summary.

- added " if statements " to catch extreme cases
- fixed function return  to formulate lists

I would change this to something like:

- Added conditional statements to only remove QARTOD variables if thresholds are unset
- Append to list of diagnostic messages returned by QC routines when masked data is detected

Amending the commit message and force pushing would be OK.

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