-
Notifications
You must be signed in to change notification settings - Fork 910
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
First try at building a parser using data from Quebec #3237
Conversation
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.
I have added some comments that needs to be fixed, but overall this is great! :)
One more thing I forgot in the first pass: timezones 😄 Here's how it could be done (inspired by # TODO: Add this after the constants
tz_obj = timezone(timedelta(hours=-4), name="GMT-4")
# TODO: Use it like this for both production and consumption
"datetime": arrow.get(elem["date"], tzinfo=tz_obj).datetime, Is the UTC offset correct and is DST something we should consider? I have asked in #3210 (comment) whether the data is in local time or not :) |
Somewhat lost on all the fancy Github features for such a large collaboration. Looks like you have automated pull requests set up, need to read up on this feature. Thanks for your support @madsnedergaard - I hope to pay it forward. |
I have implemented your suggestions, and the code runs using
Should I commit, or make the test pass first? Looking forward to more suggestions. |
… using America/Toronto
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.
Somewhat lost on all the fancy Github features for such a large collaboration. Looks like you have automated pull requests set up, need to read up on this feature. Thanks for your support @madsnedergaard - I hope to pay it forward.
Happy to help, super excited about getting Quebec on the map!
Thanks for the swift response and for fixing all the comments 💪
Just a few things left:
Maybe using just the timezone "name" (America/Montreal) is easier and could potentially(?) make sure it handles DST for us? 🤔 timezone = 'America/Montreal'
arrow.get(elem["date"], tzinfo=timezone).datetime |
I'm excited that Quebec will be on the map soon! I'd suggest changing the capacity and using the following values: Sources used: |
I've added you as a contributor to zones.json. :) |
Hi Mads, do you mean add As a constant, and using
|
I was basically thinking it could be used like |
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.
Last few comments to make CI happy and then I think we're done! 🥳
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.
👏 👏
* First try at building a parser using recently released data from Quebec, Canada * Fixes as suggested by @madsnedergaard for #3237. Fixes #3237 * Fixes time zone issues. Note, America/Montreal is not available, thus using America/Toronto * Fix time zone issues. Added more contributors * Yet anotother timezone_id fix * Removed unused imports and updated function definitions * New information on capacities and implemented special case for gas production from ticket #3218 and #3254 * Clean up of explanatory notes related to production gas in QC * Pythonic suggestion for Ternary logic from @jarek * Clear up notes realted to production, drop biomass default value of 0.0 * Clear up notes re. production and drop biomass default value of 0.0 Co-authored-by: Mads Nedergaard <nedergaardmads@gmail.com>
Hopefully this will show up on the map somehow. I look forward to seeing what changes will be made in order to make this parser functional.