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

g.gisenv: set raster compression method #287

Closed
wants to merge 8 commits into from

Conversation

metzm
Copy link
Contributor

@metzm metzm commented Jan 16, 2020

allow setting the raster compression method with g.gisenv set="GRASS_COMPRESSOR=XXX"

TODO: update manuals

allow setting the raster compression method with g.gisenv
@metzm metzm requested a review from mlennert January 16, 2020 21:46
update documentation to set GRASS_COMPRESSOR also with g.gisenv
@neteler neteler changed the title raster compression method g.gisenv: set raster compression method Jan 26, 2020
@HuidaeCho
Copy link
Member

I have two comments about this PR.

  1. Why do we need both environment and g.gisenv variables for setting the compression method? Wouldn't it confuse users? What if they are different?

  2. g.gisenv is explicitly for GRASS and no other g.gisenv variables have the GRASS_ prefix. I would drop the prefix. I know it's good to be consistent with its environment variable, but then back to comment 1.

My two cents...

@metzm
Copy link
Contributor Author

metzm commented Feb 16, 2020

@HuidaeCho The motivation for this PR comes from this thread:
"[...] compression method is defined as an environment
variable. This is somewhat daunting to many MS Windows users out there."

I like the possibility to set configuration variables in different ways as e.g. with GDAL's config options.

Of course g.gisenv is explicitly for GRASS variables, but in this case I would find it confusing to have e.g. export GRASS_COMPRESSOR=ZLIB and g.gisenv set="COMPRESSOR=ZLIB" mean the same thing.

@HuidaeCho
Copy link
Member

HuidaeCho commented Feb 24, 2020

@HuidaeCho The motivation for this PR comes from this thread:
"[...] compression method is defined as an environment
variable. This is somewhat daunting to many MS Windows users out there."

I like the possibility to set configuration variables in different ways as e.g. with GDAL's config options.

Of course g.gisenv is explicitly for GRASS variables, but in this case I would find it confusing to have e.g. export GRASS_COMPRESSOR=ZLIB and g.gisenv set="COMPRESSOR=ZLIB" mean the same thing.

We already have this inconsistency: GRASS_GUI (GUI) and GRASS_OVERWRITE (OVERWRITE). Actually, g.gisenv GRASS_GUI was renamed to g.gisenv GUI (https://lists.osgeo.org/pipermail/grass-commit/2011-July/018234.html). This commit says "gisenv variable has no
prefix". There is code for backward compatibility in grass79.py though.

Also, I found inconsistent handling of environment and g.gisenv variables for these two overlapping variables. g.gisenv OVERWRITE overrides GRASS_OVERWRITE while GRASS_GUI overrides g.gisenv GUI. Which one should take priority? I'm not sure. Are environment variables more global and should they overrule g.gisenv? Or since g.gisenv is more local (?), g.gisenv variables should override environment variables? This is why I prefer to have only one version of a variable, either as an environment or g.gisenv variable so we can avoid confusion. I cannot even tell which/when variables should be environment or g.gisenv. Are there any rules for this decision?

How does GDAL handle this situation?

@metzm
Copy link
Contributor Author

metzm commented Feb 24, 2020

We already have this inconsistency: GRASS_GUI (GUI) and GRASS_OVERWRITE (OVERWRITE). Actually, g.gisenv GRASS_GUI was renamed to g.gisenv GUI (https://lists.osgeo.org/pipermail/grass-commit/2011-July/018234.html). This commit says "gisenv variable has no
prefix". There is code for backward compatibility in grass79.py though.

I find this confusing: different variables having the same effect.

Also, I found inconsistent handling of environment and g.gisenv variables for these two overlapping variables. g.gisenv OVERWRITE overrides GRASS_OVERWRITE while GRASS_GUI overrides g.gisenv GUI. Which one should take priority? I'm not sure. Are environment variables more global and should they overrule g.gisenv? Or since g.gisenv is more local (?), g.gisenv variables should override environment variables? This is why I prefer to have only one version of a variable, either as an environment or g.gisenv variable so we can avoid confusion. I cannot even tell which/when variables should be environment or g.gisenv. Are there any rules for this decision?

Apparently there are no rules in GRASS to determine the priorities of environment and g.gisenv variables. g.gisenv in turn can read variables from gisrc or the mapset's VAR file.

Currently, handling of these variables is done individually and probably historically grown, that's why g.gisenv OVERWRITE overrides GRASS_OVERWRITE while GRASS_GUI overrides g.gisenv GUI.

How does GDAL handle this situation?

AFAIKT, GDAL does not use different variable names for the same configuration options.

GDAL first checks configuration options set by --config/CPLSetConfigOption(), then environment variables. See cpl_conv.cpp#L1661 and cpl_conv.cpp#L1718.

@HuidaeCho
Copy link
Member

HuidaeCho commented Feb 25, 2020

IMO, the priorities of these variables should be consistent and I propose:

  1. flags override everything (closest to modules)
  2. g.gisenv overrides environment variables (closer to modules and mapsets)
  3. environment variables act as a fall back when neither 1 nor 2 is given (farthest from modules).

Naming convention

  1. GRASS_ prefix for environment variables (no questions here) & no GRASS_ prefix for g.gisenv variables (g.gisenv is GRASS native, so why need a prefix?)
  2. GRASS_ prefix for both environment and g.gisenv variables (consistent naming, but more typing when using g.gisenv)

Maybe, just migrate all environment variables to g.gisenv and get rid of them for version 8 (and don't worry about the prefix)? At least to me, it's still unclear when we want to add new environment variables as compared to g.gisenv ones. Global settings for all LOCATIONs and MAPSETs vs. g.gisenv local to MAPSETs? Then, why don't we have the same version of every environment variable in g.gisenv?

@neteler
Copy link
Member

neteler commented Apr 12, 2020

Here is a conflict, can anyone please resolve it?

@HuidaeCho
Copy link
Member

Here is a conflict, can anyone please resolve it?

Resolved

lib/init/variables.html Outdated Show resolved Hide resolved
raster/rasterintro.html Outdated Show resolved Hide resolved
@neteler
Copy link
Member

neteler commented Apr 19, 2020

@metzm and @HuidaeCho: may I now merge this PR?

@HuidaeCho
Copy link
Member

@metzm and @HuidaeCho: may I now merge this PR?

I thought we were going to address inconsistency in naming in this PR (See #287 (comment)). Maybe, that's in another PR?

@metzm
Copy link
Contributor Author

metzm commented Jul 27, 2020

I thought we were going to address inconsistency in naming in this PR (See #287 (comment)). Maybe, that's in another PR?

Sorry for the late response! IMHO, the issue of variable naming should go into another PR. Some GRASS environmental variables other than those controlling raster compression methods might be transferred to GISRC variables, or mapset-specific variables. This would make life easier for users not comfortable with env vars. Ideally, a more general mechanism, maybe as in GDAL, could be introduced, where variables can be set with different methods? Obviously, the name of the variable must be the same, no matter what method is used.

@metzm metzm closed this Sep 9, 2020
@metzm metzm deleted the raster_compressor branch September 9, 2020 20:35
@neteler neteler added this to the 8.0.0 milestone Dec 9, 2021
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.

None yet

3 participants