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
Cli csv upload #85
Conversation
Closes #56 if I'm not mistaken. |
A few higher-level comments:
|
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) |
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only small improvements.
* 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>
There was a problem hiding this 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.
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. |
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 usedsession.merge
). If you think it deserves another look, I could share my faulty implementation and maybe you could take a look. Let me know.