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

Cli csv upload #85

Merged
merged 9 commits into from Apr 16, 2021
Merged

Cli csv upload #85

merged 9 commits into from Apr 16, 2021

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Apr 4, 2021

Best reviewed in conjunction with SeitaBV/timely-beliefs#54.

Note that, if the user doesn't set a horizon explicitly, the server time will be used as belief time. Running the CLI command on the same CSV file twice will then work, and ends up storing the same set of values with a different belief horizon.
If the user sets a horizon explicitly, running the CLI command on the same CSV file twice will throw a database error because the data is already there. I tried but was unsuccessful at supporting an allow_overwrite=True option, which requires an "upsert" implementation like I had done before for the API in play mode (there I used session.merge). If you think it deserves another look, I could share my faulty implementation and maybe you could take a look. Let me know.

@Flix6x Flix6x self-assigned this Apr 4, 2021
@Flix6x Flix6x requested a review from nhoening April 4, 2021 13:12
@Flix6x Flix6x mentioned this pull request Apr 4, 2021
@Flix6x
Copy link
Contributor Author

Flix6x commented Apr 4, 2021

Closes #56 if I'm not mistaken.

@nhoening
Copy link
Contributor

nhoening commented Apr 4, 2021

A few higher-level comments:

  • I like the allow_overwrite=True feature, and if you show me your attempt, I can see if I find out why it doesn't work.
  • The belief time issue also deserves a line in the CLI docstring (if you don't tell us what the horizon is, we'll assume that you now is the time you acquired this knowledge).
  • We have some code in the API to validate if the data has resolution fitting to the event resolution of the sensor, even upsampling if needed. That would be useful here, as Add datasets in CLI #56 also mentions. My plan was to factor that part out somehow (into the data package I guess) and use it on both places.

@nhoening
Copy link
Contributor

nhoening commented Apr 5, 2021

Should we maybe move the resolution check / upsampling code (see my third point) to timely-beliefs?

@Flix6x
Copy link
Contributor Author

Flix6x commented Apr 5, 2021

Should we maybe move the resolution check / upsampling code (see my third point) to timely-beliefs?

I don't think we should. This code is problematic for sensors measuring instantaneous events (such as temperature and accumulated consumption)

@Flix6x
Copy link
Contributor Author

Flix6x commented Apr 5, 2021

I made separate fm and tb branches for you to test out my attempt at upserting beliefs. See here (for FM) and here (for TB).

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only small improvements.

flexmeasures/data/scripts/cli_tasks/data_add.py Outdated Show resolved Hide resolved
Data model based on timely beliefs automation moved this from In progress to In review Apr 12, 2021
Flix6x and others added 4 commits April 13, 2021 22:22
* Try tb upsert functionality

* Try to bulk insert without overwrite (faster) before attempting to merge with overwrite (slower)

* Make overwriting data a CLI option

* Copy docstring from tb

* Simplification

* UX improvement

* Add bulk saving as class method option, but not as CLI option; change class method default to not commit transaction

* Update dependency

Co-authored-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x marked this pull request as ready for review April 13, 2021 20:45
@Flix6x Flix6x requested a review from nhoening April 13, 2021 20:46
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work.

Here is one concern: I want to avoid confusion about all this work in project 3 (timely beliefs integration / sensor & belief data remodeling). In our changelog and general communication, let's make clear that these are not features users would want to use right away, but they are part of work on a longer-term refactoring.

By the way, that project should also include the PR on adding sensors via CLI, I think.

@Flix6x
Copy link
Contributor Author

Flix6x commented Apr 14, 2021

That is a very valid concern. In addition to your suggestions, I propose to make the 2 CLI commands for adding sensors and sensor data part of a dev-add CLI group for now, to make that distinction clearer.

@Flix6x Flix6x added this to the 0.4.0 milestone Apr 16, 2021
@Flix6x Flix6x merged commit b46c78b into main Apr 16, 2021
Data model based on timely beliefs automation moved this from In review to Done Apr 16, 2021
@Flix6x Flix6x deleted the cli-csv-upload branch April 16, 2021 14:36
@nhoening nhoening mentioned this pull request Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants