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

Add extra_vars option for ts #120

Closed
wants to merge 1 commit into from
Closed

Add extra_vars option for ts #120

wants to merge 1 commit into from

Conversation

tangq
Copy link
Contributor

@tangq tangq commented Aug 18, 2021

This option is needed when regular expressions are used to specify variables for ncclimo. This makes the variable list more flexible.

For example, if the variables are specified with a mix of wildcards and variable names like
ncclimo -v area,'^DF_', it will fail.

But ncclimo -v '^DF_' --var_xtr=area will work.

This PR adds the --var_xtr option.

I tested it successfully on compy.

@tangq tangq requested a review from golaz August 18, 2021 23:22
@tangq
Copy link
Contributor Author

tangq commented Aug 18, 2021

I am not sure why some test failed. Below is the pre-commit output.
Screen Shot 2021-08-18 at 4 26 03 PM

@forsyth2
Copy link
Collaborator

It's failing on a unit test. You can run the tests at https://e3sm-project.github.io/zppy/_build/html/main/dev_guide/testing.html to see what's going wrong.

@tangq
Copy link
Contributor Author

tangq commented Aug 20, 2021

@forsyth2 , I ran the unit test and got the output below.

Because I added an option for the ts section, shall we change the unit test for it?

(zppy_dev) [ac.qtang@chrlogin2 zppy (tangq/ts_extra_vars)]$ python -m unittest tests/test_*.py
FF
======================================================================
FAIL: test_sections (tests.test_sections.TestAllSets)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/gpfs/fs1/home/ac.qtang/E3SM/code/zppy/tests/test_sections.py", line 64, in test_sections
    self.assertEqual(actual_section, expected_section)
AssertionError: {'act[272 chars]: '', 'area_nm': 'area', 'extra_vars': '', 'dpf': 30, 'tpd': 1} != {'act[272 chars]: '', 'area_nm': 'area', 'dpf': 30, 'tpd': 1}

======================================================================
FAIL: test_subsections (tests.test_sections.TestAllSets)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/gpfs/fs1/home/ac.qtang/E3SM/code/zppy/tests/test_sections.py", line 209, in test_subsections
    self.assertEqual(actual_section, expected_section)
AssertionError: {'act[252 chars]a', 'extra_vars': '', 'years': [''], 'dpf': 30[515 chars]one}} != {'act[252 chars]a', 'years': [''], 'dpf': 30, 'tpd': 1, 'ts_gr[497 chars]one}}

----------------------------------------------------------------------
Ran 2 tests in 0.036s

FAILED (failures=2)

@forsyth2
Copy link
Collaborator

Because I added an option for the ts section, shall we change the unit test for it?

Ah, yes. If you search for ts specific parameters (e..g, dpf, tpd) in https://github.com/E3SM-Project/zppy/blob/main/tests/test_sections.py, you can see where you need to insert the extra_vars parameter as well.

You're also going to want to add extra_vars = string(default=None) to the ts [[__many__]] subsection in default.ini.

@tangq
Copy link
Contributor Author

tangq commented Aug 20, 2021

@forsyth2 , I tried to change the tests on chrysalis: /home/ac.qtang/E3SM/code/zppy/tests, but it still fails and doesn't allow me to commit the change. Can you take a look?

Also, I am not familiar with the development environment (https://e3sm-project.github.io/zppy/_build/html/main/getting_started.html#b-development-environment). For E3SM code, I normally create a branch off the master and then push it to the remote and issue a PR. The zppy development procedure is different. Do I have to create a new develop environment and pre-commit install every time? Or is it a one-time thing?

Screen Shot 2021-08-20 at 10 53 07 AM

@forsyth2
Copy link
Collaborator

I tried to change the tests on chrysalis: /home/ac.qtang/E3SM/code/zppy/tests, but it still fails

extra_vars = string(default="None") in zppy/templates/default.ini should be extra_vars = string(default=None).

doesn't allow me to commit the change

This looks to me because it fails a pre-commit check. Some pre-commit checks will change files so you need git add the changed files and run the commit again. Other pre-commit checks are just suggestions and you'll have to fix the files yourself before running git add and git commit again.

For E3SM code, I normally create a branch off the master and then push it to the remote and issue a PR

zppy uses the same workflow:

> git fetch upstream
> git checkout -b <branch-name> upstream/main
# Make your changes
> git add <changed files>
> git commit
> git push <fork-name> <branch-name>
# Create pull request from your fork

Do I have to create a new develop environment and pre-commit install every time?

No, the only time you would want to create a new development environment (e.g., conda env create -f conda/dev.yml -n zppy_dev2) and install pre-commit would be if the conda files have been updated and you want the latest dependencies/conda setup. Otherwise, running pip install . is sufficient to bring in your changes to the development environment.

@forsyth2
Copy link
Collaborator

For E3SM code, I normally create a branch off the master and then push it to the remote and issue a PR

Ah, I see that your branch is actually in the main repo, not from a fork. This does work but it is not recommended, as it clutters the main repo with feature branches.

@tangq
Copy link
Contributor Author

tangq commented Aug 20, 2021

I will create a new PR from my fork to replace this one.

@tangq tangq closed this Aug 20, 2021
@tangq tangq deleted the tangq/ts_extra_vars branch August 20, 2021 21:32
@forsyth2
Copy link
Collaborator

Replaced by #124.

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