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

Adding in new function #845

Merged
merged 13 commits into from
Jul 25, 2024
Merged

Adding in new function #845

merged 13 commits into from
Jul 25, 2024

Conversation

ajsockol
Copy link
Collaborator

@ajsockol ajsockol commented Jul 17, 2024

  • Closes #xxxx
  • Tests added
  • Documentation reflects changes
  • PEP8 Standards or use of linter
  • Xarray Dataset or DataArray variable naming follows 'ds' or 'da' naming

act/utils/data_utils.py Outdated Show resolved Hide resolved
act/utils/data_utils.py Outdated Show resolved Hide resolved
act/utils/data_utils.py Show resolved Hide resolved
act/utils/data_utils.py Outdated Show resolved Hide resolved
act/utils/data_utils.py Outdated Show resolved Hide resolved
@zssherman
Copy link
Collaborator

@ajsockol Thanks for the PR! I added some review comments. Also could we add unit tests to this PR as well for the new code?

@ajsockol
Copy link
Collaborator Author

I added all the changes you mentioned except the testing. I'll add that next!

@ajsockol
Copy link
Collaborator Author

Okay, I've added the tests. Now to just figure out why the linting keeps failing

@zssherman
Copy link
Collaborator

zssherman commented Jul 17, 2024

@ajsockol Can you try installing pre-commit? It should help with the linting issues. I'm also trying to find out why the test_arm_qc tests are failing

@mgrover1
Copy link
Collaborator

mgrover1 commented Jul 17, 2024

@ajsockol Can you try installing pre-commit? It should help with the linting issues. I'm also trying to find out why the test_arm_qc tests are failing

It should be in your act_env development environment

within your main ACT directory you can run

pre-commit install 

Then, when you commit, it will auto-lint and fix where needed.

@zssherman
Copy link
Collaborator

@kenkehoe I'm still trying to familiarize with QC code of ACT, but do you possibly know why this PR might be failing the ARM QC tests?

@ajsockol
Copy link
Collaborator Author

@mgrover1 when i try to run pre-commit install . in my ACT directory (when I'm in my act_env development environment), I get this error:

Screenshot 2024-07-17 at 2 42 43 PM

@zssherman
Copy link
Collaborator

zssherman commented Jul 17, 2024

@ajsockol Maybe try pre-commit install from within the act directory no .
https://pre-commit.com/

@kenkehoe
Copy link
Contributor

@zssherman I know why the QC test is failing. We use a production DQR to test the functionality of the ARM DQR web-service. The DQR used was reprocessed so it is now not returned in the API call. I have a fix for this in my current PR. It's a simple fix if we want to include it in this PR to allow it to pass the tests.

In act.tests.qc.test_arm_qc.py change line 15 to:

ds = add_dqr_to_qc(ds, assessment='Reprocessed,Suspect,Incorrect')

@zssherman
Copy link
Collaborator

@kenkehoe Ah makes sense! Thanks for the info! We could always wait to have your PR merge in and then can rerun the tests here.

@ajsockol
Copy link
Collaborator Author

Okay I got things figured out and was able to push changes through SSH. It looks like what's failing now are the arm_qc tests

@zssherman
Copy link
Collaborator

@ajsockol Cool, yeah looks like the linter etc are passing. Ken's PR fixes that QC issue so we can wait for that one to get approved then once its merge rerun the tests here.

@ajsockol
Copy link
Collaborator Author

Okay, I copied and pasted a fix from Ken for one of the tests that was failing, but there is still one test failing from tests/discovery/test_asos.py::test_get_region that seems to be something out of my hands. @zssherman do you know what might be going wrong there?

@zssherman
Copy link
Collaborator

@ajsockol It is a connection error, i'll rerun the tests which will fix it.

@zssherman zssherman merged commit df91968 into ARM-DOE:main Jul 25, 2024
17 of 19 checks passed
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.

5 participants