Skip to content

Add pygmt.config() to change gmt defaults locally and globally - #293

Merged
weiji14 merged 8 commits into
masterfrom
gmtset
Apr 8, 2020
Merged

Add pygmt.config() to change gmt defaults locally and globally#293
weiji14 merged 8 commits into
masterfrom
gmtset

Conversation

@seisman

@seismanseisman commented Mar 13, 2019

Copy link
Copy Markdown
Member

Description of proposed changes

Support changing gmt defaults globally and locally.

Usage:

pygmt.config(PARAMETER=value) # Global
# Locally set a value that will revert back to the default
with pygmt.config(PROJ_ELLIPSOID=value):
...
pygmt.whatever(...)

Fixes#288.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

@leouieda

Copy link
Copy Markdown
Member

@seisman Looking at the linter output, I hadn't remembered that set is a builtin class in Python. Redefining it can have consequences for the other functions in the module and elsewhere. We might have to change this name to avoid unintended behavior. I'm not sure what to do because we also don't want to completely change names used in GMT. Here are a few options:

  1. Set (since it's a class, but it looks strange)
  2. configure
  3. set_parameter

I'm sure what the best option would be. I'm inclined toward pygmt.configure. Either way, we should tell pylint to stop complaining about lower case class names.

@seismanseisman changed the title WIP: Add pygmt.set() to change gmt defaults locally and globally.WIP: Add pygmt.configure() to change gmt defaults locally and globally.Mar 14, 2019
@seisman

Copy link
Copy Markdown
MemberAuthor

@leouieda What about pygmt.conf() or pygmt.config()? These two are also commonly used and are much shorter than pygmt.configure().

@leouieda

Copy link
Copy Markdown
Member

Sure, pygmt.config is explicit enough 👍

@seismanseisman changed the title WIP: Add pygmt.configure() to change gmt defaults locally and globally.WIP: Add pygmt.config() to change gmt defaults locally and globally.Mar 14, 2019
@codecov

codecovBot commented Apr 9, 2019

Copy link
Copy Markdown

Codecov Report

Merging #293 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@## master #293 +/- ##
=========================================
+ Coverage 96.74% 96.8% +0.06% 
=========================================
Files 18 18 Lines 768 783 +15 =========================================
+ Hits 743 758 +15 
Misses 25 25
Impacted FilesCoverage Δ
pygmt/modules.py100% <100%> (ø)⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b8c659...24d4d90. Read the comment docs.

@weiji14weiji14 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just following up on this from a recent gitter comment by @EqualMa. I've updated the branch to catch up with master, and have just looked into 2 of the errors:

  1. It seems that the global config doesn't turn off after we run the test, so we'll probably need a way to reset configurations back to their default. (e.g. use gmtdefaults -D or gmtget)
  2. I like the lowercase config name, but pylint doesn't like lowercase names for Python classes.

I've found Chainer's configuration settings to be a good reference. They avoid Capitalization of the name by creating a using_config function, and have that function return the Config class.

Comment threadpygmt/tests/test_config.py
Comment threadpygmt/modules.py Outdated
@tjcrone

Copy link
Copy Markdown

I've been testing pygmt.config() out locally and it appears to work. Does anyone know the status of this merge and what is left to do here? I would be happy to make a PR if there are specific issues that need to be addressed.

Without this merge, I was using gmt.conf for various settings, which does work, but as far as I can tell there is no way to get pygmt to reload gmt.conf without restarting the kernel and importing pygmt. Is there a way to get pygmt to reload the gmt.conf file without restarting the kernel? Thanks!

@weiji14

weiji14 commented Apr 1, 2020

Copy link
Copy Markdown
Member

Hi @tjcrone, functionality wise, this PR is basically done. We probably just need a minor fix that renames the class config to Config to satisfy the linter (and also change the tests to use capitalized Config). It would be nice to use lower case like in chainer here but I guess we could always wrap around that main class in a separate PR. Edit: I'll just ask pylint to ignore that one lowercase name 😄

Other than that, it would be nice to have some sort of gallery example or tutorial to highlight the use of pygmt.config. Would you like to contribute an example since you have more of a grasp on this?

@weiji14weiji14 added this to the 0.1.0 milestone Apr 1, 2020
@tjcrone

Copy link
Copy Markdown

@weiji14, I would be happy to add an example, which might go best in the user guide. It would be nice to see the docs rendered after this is merged so I can see what is needed. If this is merged will the docs render?

@weiji14weiji14 changed the title WIP: Add pygmt.config() to change gmt defaults locally and globally.Add pygmt.config() to change gmt defaults locally and globallyApr 8, 2020
@weiji14

Copy link
Copy Markdown
Member

@tjcrone, you can preview the docs by clicking on the "View deployment" button in any of our PRs (see #344 (comment)). Instructions for making a tutorial are at https://github.com/GenericMappingTools/pygmt/blob/master/CONTRIBUTING.md#tutorials, and you follow the instructions at https://github.com/GenericMappingTools/pygmt/blob/master/CONTRIBUTING.md#documentation to build the documentation locally before you push up to Github. I'll merge this in first so that pip install https://github.com/GenericMappingTools/pygmt/archive/master.zip will include pygmt.config and you can create a new branch to work on the tutorial.

Sign up for freeto 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.

Support changing gmt defaults

4 participants

@seisman@leouieda@tjcrone@weiji14