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

Options arg in read_config_user_file now optional #709

Merged
merged 7 commits into from
Jul 13, 2020

Conversation

jvegreg
Copy link
Contributor

@jvegreg jvegreg commented Jul 11, 2020

To fix and issue with the cmorizers

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
  • If writing a new/modified preprocessor function, please update the documentation
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • Please use yamllint to check that your YAML files do not contain mistakes
  • If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Closes #issue_number

bouweandela and others added 5 commits July 6, 2020 11:19
Moved the last script from esmvalcore/utils to ESMValTool
* Fixed calculation of time weights

* Fixed failing FLAKE8 test

* Added more test for time weighting and fixed cube dimensions in test
Update documentation on relative diagnostics paths and preprocessor order.

Co-authored-by: Bouwe Andela <[email protected]>
@jvegreg jvegreg requested a review from mattiarighi July 11, 2020 09:22
@mattiarighi
Copy link
Contributor

mattiarighi commented Jul 13, 2020

Can you check the failing test and the codacy issue?

@bascrezee
Copy link
Contributor

Doesn't work yet for me:

$   cmorize_obs --config-file config-ETH-LANDCLIM.yml -o CDS-SATELLITE-SOIL-MOISTURE

Traceback (most recent call last):
  File "/conda/envs/esmvaltool-public/bin/cmorize_obs", line 11, in <module>
    load_entry_point('ESMValTool', 'console_scripts', 'cmorize_obs')()
  File "/ESMValTool/esmvaltool/cmorizers/obs/cmorize_obs.py", line 175, in main
    output=run_dir, console_log_level=config_user['log_level'])
TypeError: configure_logging() got an unexpected keyword argument 'output'

@mattiarighi
Copy link
Contributor

I think is config_file wiht _

@bascrezee
Copy link
Contributor

bascrezee commented Jul 13, 2020

No, config-file seems the correct way.

@mattiarighi
Copy link
Contributor

mattiarighi commented Jul 13, 2020

It's an error at line 174 in cmorize_obs.py:

    log_files = configure_logging(
        output=run_dir, console_log_level=config_user['log_level'])

The argument output is now called output_dir. I will open a PR.

@mattiarighi mattiarighi merged commit eb78c24 into v2.0.0 Jul 13, 2020
@mattiarighi mattiarighi deleted the config_user_options_fix branch July 13, 2020 13:59
@schlunma
Copy link
Contributor

Can we merge this branch into master? I think the Bouwe's plan was to cherry-pick the necessery changes for the release into v2.0.0 after they have been merged into master.

@bascrezee
Copy link
Contributor

bascrezee commented Jul 16, 2020

Sounds like a good idea to me. I need the cmorization on a near-daily basis so would be glad if this is fixed again. Also I have two PRs in the pipeline for small additions to existing CMORizers.

@schlunma schlunma restored the config_user_options_fix branch July 16, 2020 11:41
jvegreg pushed a commit that referenced this pull request Jul 16, 2020
* Remove utils section (#697)

Moved the last script from esmvalcore/utils to ESMValTool

* Fixed bug in time weights calculation (#695)

* Fixed calculation of time weights

* Fixed failing FLAKE8 test

* Added more test for time weighting and fixed cube dimensions in test

* Avoid pytest version that crashes (#707)

* Suggested Documentation changes (#690)

Update documentation on relative diagnostics paths and preprocessor order.

Co-authored-by: Bouwe Andela <[email protected]>

* Options arg in read_config_user_file now optional

* Fix codacy warning

Co-authored-by: Bouwe Andela <[email protected]>
Co-authored-by: Manuel Schlund <[email protected]>
Co-authored-by: Steve Smith <[email protected]>
Co-authored-by: Bouwe Andela <[email protected]>
@schlunma schlunma deleted the config_user_options_fix branch July 16, 2020 11:44
@bouweandela
Copy link
Member

Can we merge this branch into master? I think the Bouwe's plan was to cherry-pick the necessery changes for the release into v2.0.0 after they have been merged into master.

Indeed, this was not the plan. Pull requests should only be merged to the master branch, never into the v2.0.0 release branch. This pull request has merged all other pull requests that were merged since the v2.0.0 feature freeze into the v2.0.0 branch. The changes were very minor, so I think it's OK to leave it as is for now, but we should be more careful about this next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants