noi-techpark / bdp-commons

GNU Affero General Public License v3.0
2 stars 12 forks source link

Fix parking forecast bug #540

Closed davidebz closed 2 years ago

davidebz commented 2 years ago

To fix please change the MainParkingForecast.java class at line 89

from

new SimpleRecordDto(timestampMillis, propertyValue, datatypeConfiguration.getPeriod()));

to

new SimpleRecordDto(forecastDatatypeTimestamp.toInstant().toEpochMilli(), propertyValue, datatypeConfiguration.getPeriod()));

Piiit commented 2 years ago

Hi @davidebz, please open a PR... (https://github.com/noi-techpark/odh-docs/wiki/Contributor-Guidelines:-Getting-started#source-code)

rcavaliere commented 2 years ago

@Piiit @dulvui please also ensure to cancel all the predictions that were imported with the deployment of this component, so that we have clean predictions in production once we deploy this

davidebz commented 2 years ago

I have send the pull request on the main branch, I hope is ok. Sincerely I have not testet the fix. Please inform us when is in production so we can immediately check that is correct

dulvui commented 2 years ago

I stopped the running data-collectors in testing and production. I will delete the data first in the testing db, check that everything is fine and then delete the data in production. Deleting data takes a lot of time (the delete queries need some hours) so we will probably be ready on Monday to merge this PR and redo the deployment with the bugfixe. @davidebz I'll let you know when the bugfix is running on testing, so you can check if everything works fine

dulvui commented 2 years ago

I deleted now all files on testing and the PR is merged and running there. @davidebz Probably tomorrow I can merge into production, so you can check

rcavaliere commented 2 years ago

@dulvui let us integrate a small time series in the testing environment so to compare the different forecasts in the time domain using analytics. Once we are sure that everything now works as expected, let's put in production (I would suggest begin of next week).

dulvui commented 2 years ago

@rcavaliere Okay that works fine

rcavaliere commented 2 years ago

@davidebz I think that the forecasts still have issues in the timestamp considered, have a look at this example:

https://analytics.opendatahub.testingmachine.eu/#%7B%22active_tab%22:0,%22height%22:%22400px%22,%22auto_refresh%22:false,%22scale%22:%7B%22from%22:%222022-06-16%22,%22to%22:%222022-06-23%22%7D,%22graphs%22:%5B%7B%22category%22:%22Parking%22,%22station%22:%22108%22,%22station_name%22:%22P08%20-%20BZ%20Centro%20via%20Mayr%20Nusser%22,%22data_type%22:%22free%22,%22unit%22:%22%22,%22period%22:%22300%22,%22yaxis%22:1,%22color%22:3%7D,%7B%22category%22:%22Parking%22,%22station%22:%22108%22,%22station_name%22:%22P08%20-%20BZ%20Centro%20via%20Mayr%20Nusser%22,%22data_type%22:%22parking-forecast-30%22,%22unit%22:%22%22,%22period%22:%221800%22,%22yaxis%22:1,%22color%22:2%7D,%7B%22category%22:%22Parking%22,%22station%22:%22108%22,%22station_name%22:%22P08%20-%20BZ%20Centro%20via%20Mayr%20Nusser%22,%22data_type%22:%22parking-forecast-60%22,%22unit%22:%22%22,%22period%22:%223600%22,%22yaxis%22:1,%22color%22:3%7D%5D%7D

ghost commented 2 years ago

You need to compare the forecasts to "occupied" instead of "free", then it looks OK:

Screenshot 2022-06-23 at 23 31 12

Actually there are no current forecasts, but that might be because it was a test.

-- Chris

rcavaliere commented 2 years ago

Ah sorry you are right, I compared the wrong type. Looks fine, @dulvui we can put this in production

dulvui commented 2 years ago

@davidebz @rcavaliere The dc is now running in production and it looks fine. Please check if everything is correct.

rcavaliere commented 2 years ago

@dulvui perfect! Now it works as expected. Thanks for your work, integration completed.