Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#2338 closed enhancement (fixed)

r.horizon rename parameters

Reported by: zarch Owned by: grass-dev@…
Priority: blocker Milestone: 7.0.0
Component: Raster Version: svn-trunk
Keywords: r.horizon, r.sun Cc:
CPU: All Platform: All

Description

What do you think if we rename the options from:

  • elev_in => elevation (like in slope_aspect);
  • horizon_step => step;
  • horizon_start => start;
  • horizon_end => end;
  • horizon => prefix or output_prefix

Attachments (1)

main.c (2.1 KB) - added by zarch 5 years ago.
Functions to get the number of decimals in a string and to return a string format given the basename

Download all attachments as: .zip

Change History (17)

comment:1 Changed 5 years ago by martinl

Milestone: 7.1.07.0.0
Priority: normalcritical

Strong +1, for elevation should be used standardized option G_OPT_R_ELEV. Milestone moved to 7.0 (renaming options should be done before releasing 7.0), see wiki:Grass7/NewFeatures#Removedmodules

comment:2 Changed 5 years ago by hcho

Component: DefaultRaster
Keywords: r.horizon added

+1. I prefer prefix over output_prefix because it's shorter.

comment:3 in reply to:  2 ; Changed 5 years ago by martinl

Replying to hcho:

+1. I prefer prefix over output_prefix because it's shorter.

note that you can shorten option as you want (keeping unique names), so 'out', 'output' or 'o_p' will work. output_prefix seems to be probably more informative... Also keeping consistency with other modules would be nice (probably good to have standardized option for modules which produce maps with given prefix/postfix?)

comment:4 Changed 5 years ago by hcho

Oh! I never knew we could use o_p.

comment:5 Changed 5 years ago by wenzeslaus

Please read #2136 and look how many modules are using particular names.

Changed 5 years ago by zarch

Attachment: main.c added

Functions to get the number of decimals in a string and to return a string format given the basename

comment:6 in reply to:  5 Changed 5 years ago by zarch

Replying to wenzeslaus:

Please read #2136 and look how many modules are using particular names.

So I change: horizon => basename, ok?

I wrote two functions, one that return the number of decimals in a string, and another one that return a format string given the basename, the number of digits and decimals that we want to use.

I attached the code with two functions plus some tests in main() [0].

Considering that these functionalities are shared between modules I think we should put them somewhere in the GARSS C API...

Where should we put them? Any ideas?

Pietro

[0] http://trac.osgeo.org/grass/attachment/ticket/2338/main.c

comment:7 in reply to:  3 ; Changed 5 years ago by neteler

Replying to martinl:

Replying to hcho:

+1. I prefer prefix over output_prefix because it's shorter.

Yes, "prefix=" like in r.texture. At least standardize it:

Also keeping consistency with other modules would be nice (probably good to have standardized option for modules which produce maps with given prefix/postfix?)

Yes, that should go into the parser.

comment:8 in reply to:  7 Changed 5 years ago by martinl

Replying to neteler:

Replying to martinl:

Replying to hcho:

+1. I prefer prefix over output_prefix because it's shorter.

Yes, "prefix=" like in r.texture. At least standardize it:

Some modules are using basename I guess. We should standardize it before releasing 7.0...

comment:9 in reply to:  7 ; Changed 5 years ago by wenzeslaus

Replying to neteler:

Replying to martinl:

Replying to hcho:

+1. I prefer prefix over output_prefix because it's shorter.

Yes, "prefix=" like in r.texture. At least standardize it:

As I was saying in in #2136: I don't like "prefix" because it is not a prefix. In my point of view, we are suffixing the analyses name or state description to the given string. I prefer "basename" because it is what it is.

comment:10 in reply to:  9 ; Changed 5 years ago by zarch

Replying to wenzeslaus:

Replying to neteler:

Yes, "prefix=" like in r.texture. At least standardize it:

As I was saying in in #2136: I don't like "prefix" because it is not a prefix. In my point of view, we are suffixing the analyses name or state description to the given string. I prefer "basename" because it is what it is.

Ok, I used basename and, to avoid repetitions, I added (r60894) two new standard options:

  • G_OPT_R_BASENAME_INPUT
  • G_OPT_R_BASENAME_OUTPUT

so If in the future we decide to change again from basename to prefix or something else, we should be able to do this modifying only in a single place.

But I realize that maybe is not enough. I modify the r.sun module to handle the changed name in r.horizon, so I used G_OPT_R_BASENAME_INPUT, in this way in r.sun the "horizon" parameter became "basename", instead I think that would be clearer to use "horizon_basename".

So what should I do? just leave "basename" or use "horizon_basename"?

Any thoughts on this?

Also in r.sun probably we should change:

  • elev_in => elevation
  • asp_in => aspect
  • aspect => ??? aspect_val? vaspect? val_aspect?
  • slope_in => slope
  • slope = ??? slope_val? vslope? val_slope?
  • linke_in => ??? linke
  • lat_in => ??? lat
  • long_in => ??? long
  • horizon => ??? basename or horizon_basename?

comment:11 in reply to:  10 ; Changed 5 years ago by annakrat

Replying to zarch:

Replying to wenzeslaus:

Replying to neteler:

Yes, "prefix=" like in r.texture. At least standardize it:

As I was saying in in #2136: I don't like "prefix" because it is not a prefix. In my point of view, we are suffixing the analyses name or state description to the given string. I prefer "basename" because it is what it is.

Ok, I used basename and, to avoid repetitions, I added (r60894) two new standard options:

  • G_OPT_R_BASENAME_INPUT
  • G_OPT_R_BASENAME_OUTPUT

so If in the future we decide to change again from basename to prefix or something else, we should be able to do this modifying only in a single place.

There should be probably also vector version but I am aware of only one module which could use that (r.sim.water). So that's not a big issue.

But I realize that maybe is not enough. I modify the r.sun module to handle the changed name in r.horizon, so I used G_OPT_R_BASENAME_INPUT, in this way in r.sun the "horizon" parameter became "basename", instead I think that would be clearer to use "horizon_basename".

So what should I do? just leave "basename" or use "horizon_basename"?

horizon_basename I would say

Any thoughts on this?

Also in r.sun probably we should change:

  • elev_in => elevation
  • asp_in => aspect
  • aspect => ??? aspect_val? vaspect? val_aspect?

aspect_value (the same is already in r.sim.water)

  • slope_in => slope
  • slope = ??? slope_val? vslope? val_slope?

slope_value

  • linke_in => ??? linke
  • lat_in => ??? lat
  • long_in => ??? long

the rest as you suggested

With the new standard option, do you think changing gisprompt would be a good idea? It could be

Opt->gisprompt = "old,cell,basename";

GUI could use that instead of guessing from the parameter name. The widget could be the standard map selection widget but when selecting the map, it could try to cut the suffix (in case we have the standardized delimiter such as __). When new series of maps are created, they could be all added (up to some number).

comment:12 in reply to:  11 Changed 5 years ago by zarch

Resolution: fixed
Status: newclosed

Replying to annakrat:

the rest as you suggested

Done in r61096.

With the new standard option, do you think changing gisprompt would be a good idea? It could be

Opt->gisprompt = "old,cell,basename";

GUI could use that instead of guessing from the parameter name. The widget could be the standard map selection widget but when selecting the map, it could try to cut the suffix (in case we have the standardized delimiter such as __). When new series of maps are created, they could be all added (up to some number).

+1

yes I think that add a basename option to gisprompt could be a good idea.

I close this ticket, we could discuss further about this in #2136.

comment:13 Changed 5 years ago by neteler

Keywords: r.sun added
Resolution: fixed
Status: closedreopened

Reopening as r61098 + r61099 are not yet backported to relbr7 (parameter name changes)

comment:14 Changed 5 years ago by neteler

Priority: criticalblocker

TODO: Besides backport check also manual page examples

comment:15 Changed 5 years ago by neteler

Resolution: fixed
Status: reopenedclosed

r.horizon and r.sun parameter changes:

Backported r61096 + r61098 + r61099 in r62430, Closing.

comment:16 in reply to:  15 Changed 5 years ago by neteler

Replying to neteler:

r.horizon and r.sun parameter changes:

Backported r61096 + r61098 + r61099 in r62430, Closing.

for the record: completed in r62473.

Note: See TracTickets for help on using tickets.