[GRASS-dev] [GRASS GIS] #2876: r.sun (mode 1): data are not timestamped correctly

#2876: r.sun (mode 1): data are not timestamped correctly
----------------------------+-------------------------
Reporter: fabriziosossan | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone:
Component: Raster | Version: 6.4.5
Keywords: r.sun | CPU: x86-64
Platform: Linux |
----------------------------+-------------------------
While comparing the output of r.sun (mode 1) with experimentally measured
data from a pyranometer, we noticed that the produced global horizontal
irradiance data are not correctly timestamped.

The problem was given by a wrong sign in the code. Please see the attached
patch.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2876&gt;
GRASS GIS <https://grass.osgeo.org>

#2876: r.sun (mode 1): data are not timestamped correctly
-----------------------------+-------------------------
  Reporter: fabriziosossan | Owner: grass-dev@…
      Type: defect | Status: new
  Priority: normal | Milestone:
Component: Raster | Version: 6.4.5
Resolution: | Keywords: r.sun
       CPU: x86-64 | Platform: Linux
-----------------------------+-------------------------
Changes (by fabriziosossan):

* Attachment "patch.txt" added.

Patch for the file grass-6.4.5/raster/r.sun2/main.c

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2876&gt;
GRASS GIS <https://grass.osgeo.org>

#2876: r.sun (mode 1): data are not timestamped correctly
-----------------------------+-------------------------
  Reporter: fabriziosossan | Owner: grass-dev@…
      Type: defect | Status: new
  Priority: normal | Milestone: 7.0.3
Component: Raster | Version: 6.4.5
Resolution: | Keywords: r.sun
       CPU: x86-64 | Platform: Linux
-----------------------------+-------------------------
Changes (by wenzeslaus):

* milestone: => 7.0.3

Comment:

Formally it looks good to me. Can you please comment on it, so we can put
it to the source code with explanation?

And slightly related to that, are you willing to share some of your
results? There are many options how to do that depending on your methods,
but a very interesting one is an actual Python code which does the
comparison.

For those who are in doubts: r.sun2 in 6.4 branch is r.sun in 7.0 branch.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2876#comment:1&gt;
GRASS GIS <https://grass.osgeo.org>

#2876: r.sun (mode 1): data are not timestamped correctly
-----------------------------+-------------------------
  Reporter: fabriziosossan | Owner: grass-dev@…
      Type: defect | Status: new
  Priority: normal | Milestone: 7.0.4
Component: Raster | Version: 6.4.5
Resolution: | Keywords: r.sun
       CPU: x86-64 | Platform: Linux
-----------------------------+-------------------------

Comment (by fabriziosossan):

Hi,

This is our logic explanation for changing the time offset sign from
positive to negative.

It should be noted first that there is no agreed convention for the sign
of the Equation of Time (EoT in the following) and can appear in both the
forms shown in [1] and [2]. The EoT implemented in the code is as former,
where positive y values denotes that a wall clock is slower than a
sundial, and vice-versa.
For example, assuming to be i) at greenwich latitude ii) on the 1st
January (when EoT is approx. -3 minutes) and iii) with the sun exactly at
south (solar noon), a wall clock would appear faster and would mark 12:03
of civil time. Therefore the additive offset to pass from the solar time
to civil time should be positive, namely the EoT expression should be with
inverted sign.

Inverting the EoT sign in order to obtain, for example, a positive offset
for the 1st of January, is coherent with the correction that is applied
for the longitude displacement later in code:

longitTime = -longitude / 15.

In this case, when moving eastwise from the greenwich meridian (this means
progressively increasing positive values of the longitude), the sun
position will move in the sky to west: in this case the clock would
appear slower than a sundial and the offset, conversely to the previous
case, should be indeed strictly negative.

Finally, one might wonder if later in the code the signs of the discussion
expressions are changed: this is not the case as these two contributions
are summed up together.

[1]
https://en.wikipedia.org/wiki/Equation_of_time#/media/File:Equation_of_time.svg
[2] https://en.wikipedia.org/wiki/Equation_of_time#/media/File
:Tijdvereffening-equation_of_time-en.jpg

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2876#comment:4&gt;
GRASS GIS <https://grass.osgeo.org>

#2876: r.sun (mode 1): data are not timestamped correctly
-----------------------------+-------------------------
  Reporter: fabriziosossan | Owner: grass-dev@…
      Type: defect | Status: new
  Priority: normal | Milestone: 7.0.4
Component: Raster | Version: 6.4.5
Resolution: | Keywords: r.sun
       CPU: x86-64 | Platform: Linux
-----------------------------+-------------------------

Comment (by martinl):

Is there any plan for backport? It's noted in
wiki:Grass7Planning#a7.0.4tobebackported and we are close to RC1 (15/4)...

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2876#comment:5&gt;
GRASS GIS <https://grass.osgeo.org>

I presume that by “backport” you mean port of this fix to GRASS7?

On 28 February I reported bug #2941 with a suggested patch to Grass 7 to cover this. Since I’m not a regular contributor, I don’t really know what has happened to it?

It’s just a one-liner, so maybe it would be safest if one of the regulars would apply the patch?

Thomas

···

On Tue, Apr 5, 2016 at 11:17 AM, GRASS GIS <trac@osgeo.org> wrote:

#2876: r.sun (mode 1): data are not timestamped correctly
-----------------------------±------------------------
Reporter: fabriziosossan | Owner: grass-dev@…
Type: defect | Status: new
Priority: normal | Milestone: 7.0.4
Component: Raster | Version: 6.4.5
Resolution: | Keywords: r.sun
CPU: x86-64 | Platform: Linux
-----------------------------±------------------------

Comment (by martinl):

Is there any plan for backport? It’s noted in
wiki:Grass7Planning#a7.0.4tobebackported and we are close to RC1 (15/4)…


Ticket URL: <https://trac.osgeo.org/grass/ticket/2876#comment:5>
GRASS GIS <https://grass.osgeo.org>


grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Thomas,

On Tue, Apr 5, 2016 at 8:55 PM, Thomas Huld <thomas.huld@gmail.com> wrote:

I presume that by "backport" you mean port of this fix to GRASS7?

Yes, to fix 7.0.x.

On 28 February I reported bug #2941 with a suggested patch to Grass 7 to
cover this. Since I'm not a regular contributor, I don't really know what
has happened to it?

So far nothing because it is unclear to me (as written in the ticket).

It's just a one-liner, so maybe it would be safest if one of the regulars
would apply the patch?

Shall I apply one of them or both?
https://trac.osgeo.org/grass/ticket/2876
https://trac.osgeo.org/grass/ticket/2941

or does the second also include the first? Please let me know,

Markus

Hi all,

I’m interested on this issue. Now I’m working on grass 7.0.3 if I want to set civili time of Central Europe which sign shall I use +1 or -1.

Tnx

Lorenzo

···

2016-04-11 23:15 GMT+02:00 Markus Neteler <neteler@osgeo.org>:

Thomas,

On Tue, Apr 5, 2016 at 8:55 PM, Thomas Huld <thomas.huld@gmail.com> wrote:

I presume that by “backport” you mean port of this fix to GRASS7?

Yes, to fix 7.0.x.

On 28 February I reported bug #2941 with a suggested patch to Grass 7 to
cover this. Since I’m not a regular contributor, I don’t really know what
has happened to it?

So far nothing because it is unclear to me (as written in the ticket).

It’s just a one-liner, so maybe it would be safest if one of the regulars
would apply the patch?

Shall I apply one of them or both?
https://trac.osgeo.org/grass/ticket/2876
https://trac.osgeo.org/grass/ticket/2941

or does the second also include the first? Please let me know,

Markus


grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

On 11/04/16 23:15, Markus Neteler wrote:

Thomas,

On Tue, Apr 5, 2016 at 8:55 PM, Thomas Huld <thomas.huld@gmail.com> wrote:

I presume that by "backport" you mean port of this fix to GRASS7?

Yes, to fix 7.0.x.

On 28 February I reported bug #2941 with a suggested patch to Grass 7 to
cover this. Since I'm not a regular contributor, I don't really know what
has happened to it?

So far nothing because it is unclear to me (as written in the ticket).

It's just a one-liner, so maybe it would be safest if one of the regulars
would apply the patch?

Shall I apply one of them or both?
https://trac.osgeo.org/grass/ticket/2876
https://trac.osgeo.org/grass/ticket/2941

or does the second also include the first? Please let me know,

Actually the both address the same issue, but the first is supposed to be for grass64 and the second for grass7. However, the code is the same in both versions of r.sun, but the patches solve the problem in two different places (the first a few lines down from the second). Why the difference and what is the preferred solution ?

Moritz

Hi Lorenzo,

The time zone is +1 for Central Europe.

Cheers,

Thomas

···

On Tue, Apr 12, 2016 at 9:39 AM, Lorenzo Bottaccioli <lorenzo.bottaccioli@gmail.com> wrote:

Hi all,

I’m interested on this issue. Now I’m working on grass 7.0.3 if I want to set civili time of Central Europe which sign shall I use +1 or -1.

Tnx

Lorenzo

2016-04-11 23:15 GMT+02:00 Markus Neteler <neteler@osgeo.org>:

Thomas,

On Tue, Apr 5, 2016 at 8:55 PM, Thomas Huld <thomas.huld@gmail.com> wrote:

I presume that by “backport” you mean port of this fix to GRASS7?

Yes, to fix 7.0.x.

On 28 February I reported bug #2941 with a suggested patch to Grass 7 to
cover this. Since I’m not a regular contributor, I don’t really know what
has happened to it?

So far nothing because it is unclear to me (as written in the ticket).

It’s just a one-liner, so maybe it would be safest if one of the regulars
would apply the patch?

Shall I apply one of them or both?
https://trac.osgeo.org/grass/ticket/2876
https://trac.osgeo.org/grass/ticket/2941

or does the second also include the first? Please let me know,

Markus


grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

On Tue, Apr 12, 2016 at 8:47 AM, Thomas Huld <thomas.huld@gmail.com> wrote:

The time zone is +1 for Central Europe.

Is somebody willing to contribute a table for the manual with examples of
time zones and values needed?

On Tue, Apr 12, 2016 at 12:08 AM, Thomas Huld <thomas.huld@gmail.com> wrote:

Markus,

The two patches do the same thing, change the sign of the correction of the
time according to the equation. I thought my version (#2941) was a bit
clearer. The only other difference is the line number to which the patch is
applied, #2876 is for GRASS 6.4.x, #2941 for GRASS 7.0.x.

Does this make it clearer?

Cheers,

Thomas

Thanks, Thomas. We shall decide now based on your comment.

Markus

On Tue, Apr 12, 2016 at 2:56 PM, Vaclav Petras <wenzeslaus@gmail.com> wrote:

On Tue, Apr 12, 2016 at 8:47 AM, Thomas Huld <thomas.huld@gmail.com> wrote:

The time zone is +1 for Central Europe.

Is somebody willing to contribute a table for the manual with examples of
time zones and values needed?

Do you mean something like this?
https://en.wikipedia.org/wiki/List_of_UTC_time_offsets
https://en.wikipedia.org/wiki/List_of_tz_database_time_zones

Markus

#2876: r.sun (mode 1): data are not timestamped correctly
-----------------------------+-------------------------
  Reporter: fabriziosossan | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 7.0.4
Component: Raster | Version: 6.4.5
Resolution: fixed | Keywords: r.sun
       CPU: x86-64 | Platform: Linux
-----------------------------+-------------------------
Changes (by neteler):

* status: new => closed
* resolution: => fixed

Comment:

Thanks for testing r.sun and the patch, applied in r68253 in
releasebranch_6_4.

Compare also the equivalent alternative GRASS GIS 7 fix suggested in #2941
(commented on in cited email in
https://lists.osgeo.org/pipermail/grass-dev/2016-April/079789.html).

Closing.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2876#comment:6&gt;
GRASS GIS <https://grass.osgeo.org>

On Tue, Apr 12, 2016 at 4:55 PM, Markus Neteler <neteler@osgeo.org> wrote:

On Tue, Apr 12, 2016 at 12:08 AM, Thomas Huld <thomas.huld@gmail.com> wrote:

Markus,

The two patches do the same thing, change the sign of the correction of the
time according to the equation. I thought my version (#2941) was a bit
clearer. The only other difference is the line number to which the patch is
applied, #2876 is for GRASS 6.4.x, #2941 for GRASS 7.0.x.

Does this make it clearer?

Yes. In order to make progress here and close the issues, I have
applied the patches as suggested in the respective tickets:

* r68253 in releasebranch_6_4 from #2876
* r68254 (relbranch70) and r68255 (trunk = 7.12.svn).

thanks to the contributors,
Markus

--
Markus Neteler
http://www.mundialis.de
http://courses.neteler.org/blog/

On Tue, Apr 12, 2016 at 3:59 PM, Markus Neteler <neteler@osgeo.org> wrote:

On Tue, Apr 12, 2016 at 4:55 PM, Markus Neteler <neteler@osgeo.org> wrote:

On Tue, Apr 12, 2016 at 12:08 AM, Thomas Huld <thomas.huld@gmail.com> wrote:

Markus,

The two patches do the same thing, change the sign of the correction of the
time according to the equation. I thought my version (#2941) was a bit
clearer. The only other difference is the line number to which the patch is
applied, #2876 is for GRASS 6.4.x, #2941 for GRASS 7.0.x.

Does this make it clearer?

Yes. In order to make progress here and close the issues, I have
applied the patches as suggested in the respective tickets:

* r68253 in releasebranch_6_4 from #2876
* r68254 (relbranch70) and r68255 (trunk = 7.12.svn).

Just wondering, do we have any idea how much this changes the results?

Anna

thanks to the contributors,
Markus

--
Markus Neteler
http://www.mundialis.de
http://courses.neteler.org/blog/
_______________________________________________
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev