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

Temporal controller monthly time step uses 30 days instead of a month to loop #37829

Closed
RousseauLambertLP opened this issue Jul 15, 2020 · 14 comments
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Feedback Waiting on the submitter for answers Temporal Temporal filtering or animation

Comments

@RousseauLambertLP
Copy link

RousseauLambertLP commented Jul 15, 2020

How to Reproduce

  1. Add a WMS layer with a monthly time step, for example you can use https://geo.weather.gc.ca/geomet?LAYER=CANSIPS.MEM.ETA_TT.10
  2. In the layer panel, open the layer properties and open the temporal feature. Make sure the values are: Start date: 2013-05-01 00:00:00 / End date: 2018-10-01 00:00:00
  3. Open the Temporal controller in the animated temporal navigation and hit the refresh button, then change time step to 1 month
  4. Click the go to next frame button and you will see that it looks like it loops over 30 days instead of a full month. For the layer above here are some of the time steps I get: 2013-05-01 00:00:00, 2013-05-31 00:00:00, 2013-06-30 00:00:00, 2013-07-30 00:00:00, 2013-08-29 00:00:00, etc

QGIS and OS versions

QGIS version 3.15.0-Master QGIS code revision 2e2acf9
Compiled against Qt 5.11.2 Running against Qt 5.11.2
Compiled against GDAL/OGR 3.2.0dev Running against GDAL/OGR 3.2.0dev
Compiled against GEOS 3.8.1-CAPI-1.13.3 Running against GEOS 3.8.1-CAPI-1.13.3
Compiled against SQLite 3.29.0 Running against SQLite 3.29.0
PostgreSQL Client Version 11.5 SpatiaLite Version 4.3.0
QWT Version 6.1.3 QScintilla2 Version 2.10.8
Compiled against PROJ 7.2.0 Running against PROJ Rel. 7.2.0, November 1st, 2020
OS Version Windows 10 (10.0) This copy of QGIS writes debugging output.
Active python plugins openlayers_plugin; qgis2web; qgisnetworklogger; quick_map_services; tile_plus; db_manager; MetaSearch

Additional context

We have seen this behavior for every monthly time interval WMS layer we have tested in the tool, which result in layers not been displayed.

CC @tomkralidis

@RousseauLambertLP RousseauLambertLP added the Bug Either a bug report, or a bug fix. Let's hope for the latter! label Jul 15, 2020
@gioman gioman added the Temporal Temporal filtering or animation label Jul 15, 2020
@rduivenvoorde
Copy link
Contributor

rduivenvoorde commented Nov 29, 2020

@RousseauLambertLP I'm thinking about having a look at @dminor 's pull request, but was wondering about some edge cases like:

  • what if somebody chooses 30th of januari, what will be the next timestep then (given februari only has 28 days)?
  • or more general 31th of a month (half of the months do not have a 31th day), what about that?
    Do you have a good idea for those cases?
    Like 'remember the actual number, but in months wich do not have a 31th day, take 30?
    Or.. if there is not a 31th (or 30, 29th) day, take the 30days (average) for that step (but remember the actual date and use it if it is possible?
    Or.. make it not possible to choose 'month' in case the date is 29,30 or 31?
    Or.. just request 31th of february (and and let the server decide what to do: error out or sent an alternative?)
    I tried to test the wms example you give, but apparently it has a problem as I receive errors all the time (non time related)?

@Samweli
Copy link
Contributor

Samweli commented Nov 30, 2020

  • what if somebody chooses 30th of januari, what will be the next timestep then (given februari only has 28 days)?

@rduivenvoorde I think from user perceptive, the next timestep should default to the last day of the next month(for this case it ll be 28th February)

Samweli pushed a commit to Samweli/QGIS that referenced this issue Dec 22, 2020
Currently, we pass the frame duration as a QgsInterval and use the average
duration of a month or year during the animation, for instance, 30 days
rather than a month. This makes it impossible to have an animation that
displays on a particular day each month, as the day in the next month will
change depending on the number of days in the previous month.

This changes QgsTemporalNavigationObject to take the time step and time
step unit as separate arguments. The settings in
QgsTemporalUtils::exportAnimation are left unchanged, because in this case
the user interface is already set up to use an interval.

If the time step has a fractional value, the frame duration is calculated
using a QgsInterval as before. If it has an integer value, the calculation
uses QDateTime to advance by the specified time step instead. So a value of
1.5 months results in a frame duration of 45 days, but a value of 1 month
will result in a duration that depends on the length of the current month.

Fixes qgis#37829.
nyalldawson pushed a commit to nyalldawson/QGIS that referenced this issue Jan 5, 2021
Currently, we pass the frame duration as a QgsInterval and use the average
duration of a month or year during the animation, for instance, 30 days
rather than a month. This makes it impossible to have an animation that
displays on a particular day each month, as the day in the next month will
change depending on the number of days in the previous month.

This changes QgsTemporalNavigationObject to take the time step and time
step unit as separate arguments. The settings in
QgsTemporalUtils::exportAnimation are left unchanged, because in this case
the user interface is already set up to use an interval.

If the time step has a fractional value, the frame duration is calculated
using a QgsInterval as before. If it has an integer value, the calculation
uses QDateTime to advance by the specified time step instead. So a value of
1.5 months results in a frame duration of 45 days, but a value of 1 month
will result in a duration that depends on the length of the current month.

Fixes qgis#37829.
@rduivenvoorde
Copy link
Contributor

@Samweli @nyalldawson are we sure this is to be closed? In my test here with the url of the original issuer:

https://geo.weather.gc.ca/geomet?LAYER=CANSIPS.MEM.ETA_TT.10

Things work initially great. You start on the first of the month and you step through all 'firsts' of the upcoming months.

But it get heary when you set the timestep to 1 day, go to some day (like the 23th), then change to 30 days, which will by QGIS converted to 1 month(!), and then (seeing "timestep 1 month") QGIS steps 30 days again?

Also: because of the code that tries to guess what I want (if I set 7 days, make it a week etc etc), QGIS makes it harde to actually SET the start/end of the timestep.

Would it not be better to let the user really decide in what kind of steps the controller is going (like: if I want it 7 days, do not make it a week or so?)

I do understand this is getting rather complex... so don't get me wrong :-(
(I tried to make a screencast.. but failed)

@RousseauLambertLP @dminor can you confirm?

@rduivenvoorde rduivenvoorde reopened this Jan 6, 2021
@Samweli
Copy link
Contributor

Samweli commented Jan 6, 2021

then change to 30 days, which will by QGIS converted to 1 month(!), and then (seeing "timestep 1 month") QGIS steps 30 days again?

@rduivenvoorde I confirm this on master and QGIS 3.16

are we sure this is to be closed?

I think this is closed, we may need to open another ticket to address the timestep setting issue.

@rduivenvoorde
Copy link
Contributor

Sure? so 2 tickets?

@Samweli
Copy link
Contributor

Samweli commented Jan 6, 2021

so 2 tickets?

no , only one ticket.

  • one which addresses the 'guessing' behavior ( I think it is this

this is the only ticket that should be opened, a fix for this should also fix other problems as this issue is the root of the other misbehaviour.

@Samweli
Copy link
Contributor

Samweli commented Jan 7, 2021

ping @rduivenvoorde

@rduivenvoorde
Copy link
Contributor

rduivenvoorde commented Jan 7, 2021

@Samweli I created: #40871 yesterday
Though maybe not clear enough this is about the 'guessing' stuff?

@Samweli
Copy link
Contributor

Samweli commented Jan 7, 2021

I see @rduivenvoorde, but I think #40871 is about another issue, let me open a ticket about the 'guessing' stuff.

@Samweli
Copy link
Contributor

Samweli commented Jan 7, 2021

@rduivenvoorde see #40894

@rduivenvoorde
Copy link
Contributor

@RousseauLambertLP can you please test. I think this one can be closed.

@rduivenvoorde rduivenvoorde added the Feedback Waiting on the submitter for answers label Feb 12, 2021
@github-actions
Copy link

The QGIS project highly values your report and would love to see it addressed. However, this issue has been left in feedback mode for the last 14 days and is being automatically marked as "stale".
If you would like to continue with this issue, please provide any missing information or answer any open questions. If you could resolve the issue yourself meanwhile, please leave a note for future readers with the same problem and close the issue.
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this issue.
If there is no further activity on this issue, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Feb 27, 2021
@gioman
Copy link
Contributor

gioman commented Jun 4, 2021

Closing for lack of feedback.

@gioman gioman closed this as completed Jun 4, 2021
@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jun 4, 2021
@RousseauLambertLP
Copy link
Author

Sorry for not replying, I was in parental leave for the last 8 months.

I tested QGIS 3.20 temporal animation tool using https://geo.weather.gc.ca/geomet?LAYER=CANSIPS.MEM.ETA_TT.10 and the monthly time step (default time step for this WMS layer) and I see the correct behavior for this layer.
The layer loops on the months and not 30 days, which was the issue here.

Thanks all for this work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Feedback Waiting on the submitter for answers Temporal Temporal filtering or animation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants