Add period filter to time series catalogue + make sure that start & end are updated based on selected locations

lexvand commented 4 years ago

EDIT: While working on this ticket. Also have a look at this bug: https://github.com/nens/lizard-catalogue/issues/231

Design: https://xd.adobe.com/view/2ca9bb91-e3f0-45c8-bc17-a0f15a4dfefc-79fa/screen/dfa6c937-94a2-46ee-b452-6392908e42b2

Next to a filter on observation type we would like to add a filter on period. Based on the defined period the locations on the map should appear or disappear when they have data within that period.

Based on the selected locations the start and end within step 3 should be updated according to:

start = location with the earliest start date
end= location with the last end date. That way we make sure that all data is exported er opened in the portal.

hoanphungt commented 3 years ago

@Derryrover Hi Tom,

Could you review this PR: https://github.com/nens/lizard-catalogue/pull/234? This PR also fixes the bug in https://github.com/nens/lizard-catalogue/issues/231.

Kr, Hoan

Derryrover commented 3 years ago

@hoanphungt Looks good ready to merge!

GeOdin commented 3 years ago

@hoanphungt @Derryrover

Hey Hoan and Tom :)

Nice that you now get feedback when the start time was after the end time. :) When the start time is before the end time, the portal shows the same timespan.

I noticed 4 things. Let me know if that is as designed or whether it becomes part of this ticket or a next ticket.

Kind regards, Madeleine

1. start and end time not the same in catalogue and portal if start === end time

When you select the same start and end time in the catalogue and press 'SHOW IN PORTAL', the start and end time are no longer the same. The screenshots below are for the same start and end time of '30/12/2020 00:00'.

2. invalid date time possible (int/ float)

Values that are not valid date time values are possible. You seem to be able to enter integers and floats in the date time box for start and end time. When entering strings in the start time, the 'Export timeseries' buttons becomes greyed out. However, when entering a date in the start time and a string in the end time, the 'Export timeseries' button does not become greyed out.

3. size int matters for grey out timeseries button

An integer or float that has 5 numbers (before the comma) do not grey out the export timeseries button.
An integer or float that has more than 5 number (before the comma) do grey out the export timeseries button.

4. Consistency in error feedback?

When there is no start time, the export timeseries button is greyed out. I was wondering...

Would it be more consistent to also grey out buttons if the start time is before the end time? A red error message below the start and end time is shown now.
Or show an error message if no start time is set? The 'Export timeseries' button is now greyed out.

GeOdin commented 3 years ago

@Derryrover @hoanphungt

Hey guys, I also found this. Is this what is supposed to happen?

Kind regards, Madeleine

5. Export timeseries also does not seem to work.

I am not sure if export timeseries worked previously. I tried to export timeseries with the export timeseries for an asset that has values (checked this on the lizard staging client). But there are no values when I download the timeseries via the catalogue (De Bilt 06260).

Example below:

hoanphungt commented 3 years ago

@GeOdin Regarding your bullet number 5, there is no values when you export the timeseries which is the correct behaviour. It happened because of your period filter. From 01/01/2019 to 31/01/2019, there is no timeseries with data during this period as you can also see in the map, there is no locations on the map. If you change the start date to 01/01/2016 for example, then you will see values in the spreadsheet.

hoanphungt commented 3 years ago

@GeOdin
Hi Madeleine,

Thanks for your findings. I made this PR: https://github.com/nens/lizard-catalogue/pull/240 to address these issues and made another staging release to test the fixes. Regarding these issues, I addressed them as follows:

start and end time not the same in catalogue and the portal when start === end:

This is because in the portal only the date is taken into account, not the time, so the time input doesn't matter but only the date. The format that I used to open the timeseries in the portal is MMM,DD,YYYY-MMM,DD,YYYY, so for example: Jan,20,2020-Jan,21,2020. @lexvand Do you think this is sufficient?

Invalid date/time input (integer/float/characters):

In the PR, I made a quick fix for it so they become invalid in this case (but not when you enter an integer with less than 6 number). I used moment library and for integer with less than 6 number, it can be translated to a date value.

size int matters for grey out timeseries button: as mentioned earlier, the moment library can translate integer with less than 6 number to a date value
Consistency in error feedback:

I improved the error feedback and disabled button by:

Disabled timeseries export button when there is error feedback with help text when hover over the button (i.e. invalid date inputs, start date is after end date, no location/timeseries is selected)
Disabled timeseries export button when no start date is provided with help text when hover over the button (help text = Please enter a start date to export timeseries). The reason that I don't provide an error message here is because sometimes, you don't want to use the period filter to export timeseries but only to view the timeseries. In this case, no need to enter a start date.

As I explained in the previous comment, it is not a bug but happened due to your period filter.

Could you have a look again on staging to see if the fixes are good enough?

Kind regards Hoan

GeOdin commented 3 years ago

@hoanphungt

Hey Hoan :)

Thank you for your improvements. It looks good!

Kind regards, Madeleine

lexvand commented 3 years ago

I'm afraid the filter can't work correctly based on only AND filtering in the API. I made a drawing to illustrate the issue.

These 4 periods should result in a match with the given timeseries object. Currently we only support number 4, which is quite an unlikely use case. The logic would require an OR statement to make different combinations of correct filters comparing tsstart with filter_start and filter_end and comparing tsend with filter_start and filter_end.

We need another filter in the API to support this feature, which does an intersect. It requires more logic in the backend. A quick fix to make it more useful in the meantime is to flip the signs, meaning: timeseries__start__lte=2019-01-01T00:00:00Z&timeseries__end__gte=2021-01-27T00:00:00Z instead of timeseries__start__gte=2019-01-01T00:00:00Z&timeseries__end__lt=2021-01-27T00:00:00Z

@hoanphungt Could you fix that?

lexvand commented 3 years ago

@hoanphungt Discussed with Tom en he had experience with this type of queries and it is possible to do it this way. It should become: timeseries__start__lte=2021-01-27T00:00:00Z&timeseries__end__gte=2019-01-01T00:00:00Z instead of timeseries__start__gte=2019-01-01T00:00:00Z&timeseries__end__lt=2021-01-27T00:00:00Z

hoanphungt commented 3 years ago

@lexvand @Derryrover There will be 2 cases though:

Only start date is filled in: for example: timeseries__start__gte=2019-01-01T00:00:00Z. Should I also flip the sign in this case using the current moment as start? So for example:

timeseries__start__lte=2021-01-29T11:24:00Z&timeseries__end__gte=2019-01-01T00:00:00Z

Both start date and end date are filled in: is it correct that I would always flip the sign in this case to ensure all the 4 cases are covered? Then the query would become:

timeseries__start__lte=2021-01-27T00:00:00Z&timeseries__end__gte=2019-01-01T00:00:00Z

Btw, I can't figure what lt and gte in this case mean and why did you change the position of lt and gte in the above query? :)

EDIT: I got the answer for the 2nd question. Only the 1st question left.

GeOdin commented 3 years ago

@hoanphungt

Hey Hoan :)

Could it be that lt stands for where the value of the field is less than (i.e. <) the specified value? https://docs.mongodb.com/manual/reference/operator/query/lt/

Could it be that gte stands for where the value of the field is greater than or equal to (i.e. >=) a specified value (e.g. value.)? https://docs.mongodb.com/manual/reference/operator/query/gte/

Kind regards, Madeleine

hoanphungt commented 3 years ago

@GeOdin Ah nice! Yes, it seems the correct explanation of those terms. Also, lte means less than or equal to as well.

In this case, I think my 2nd question is already answered. Only my 1st question left whether I should also flip the sign if only start date is filled in using the current moment.

hoanphungt commented 3 years ago

@lexvand Seems like for case number 1, only start date is filled in, if I flip the sign and use the current moment as start date like this query:

https://nxt3.staging.lizard.net/api/v4/monitoringnetworks/c6ef3248-e8c2-47b4-9c03-afe90d6ac9be/locations/?page_size=10000&timeseries__start__lte=2021-01-29T11:25:45Z&timeseries__end__gte=2018-07-01T00:00:00Z

It would return more results than the old query:

https://nxt3.staging.lizard.net/api/v4/monitoringnetworks/c6ef3248-e8c2-47b4-9c03-afe90d6ac9be/locations/?page_size=10000&timeseries__start__gte=2018-07-01T00:00:00Z

So I will also flip the sign in this case. One more question, the time passed in the query here should also be converted to UTC time, is it correct? Currently, it added the local time to the query. I am going to convert it to UTC before adding it to the query.

lexvand commented 3 years ago

@hoanphungt Case 1: If only start is filled in the query becomes: timeseriesendgte={filled in start}. Case 2: If only end is filled in the query becomes: timeseriesstartlte={filled in end}. Case 3: If both are filled in the query becomes: timeseriesstartlte={filled in end}&timeseriesendgte={filled in start}.

May sound counterintuitive, but the logic is correct.

hoanphungt commented 3 years ago

Ok, sounds good!

hoanphungt commented 3 years ago

@Derryrover Hi Tom

Could you review this PR: https://github.com/nens/lizard-catalogue/issues/251?

Steps to test:

Select time-series in the lizard catalogue
Select KNMI All monitoring network
Open the Time-Series selection modal
Select filters for the locations for 3 cases:

Only start date = 01/07/2018, no end date
Only end date = 01/01/2021, no start date
Enter both start and end dates: from 01/07/2018 to 01/01/2021

Kr, Hoan

Derryrover commented 3 years ago

Looks good you can merge !

lexvand commented 3 years ago

@hoanphungt Not yet on staging, right? Moved it to this new pipeline (see description for purpose of pipeline).

nens / lizard-catalogue