-
Notifications
You must be signed in to change notification settings - Fork 903
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
Update zones.json #4290
Update zones.json #4290
Conversation
Updated installed capacity for India as of May 22.
@gopikrishna1793 I see you opened a new PR, this is not needed and it would be best if you updated the original one (#4289). I'm gonna leave this open for now but once the changes requested are made in the other PR, #4289, I'm going to merge that and close this. And as I said on the other PR, if you need help with anything just let me know and I'll assist. |
Removed comments in zones.json
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 reviewed everything and there are quite a few changes that has to be made, I have made suggestions for most of them that you can just accept but there are one or two you have to do manually. (If you want I can apply the suggestions.)
Conversation originally started in: #4289
config/zones.json
Outdated
"solar": 5518.153, | ||
"wind": 8618.070 | ||
}, | ||
"comment": "https://tnebsldc.org/reports1/peakdet.pdf", |
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.
Can you move this source to `DATA_SOURCES.md too?
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.
Moved this link to DATA_SOURCES.md under realtime source. The URL remains the same but the data for daily consumption varies in this same file. Using beautifulsoup and python sricpt can fetch this data.
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.
Hmm I can't seem to open that PDF file.
However it should only be in realtime sources if it's used to fetch electricity production/consumption not capacity.
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.
tnebsldc . This is the URL I referred to. It has the capacity and consumption. Solar generation is also mentioned at the bottom line.
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.
Yeah it's not loading for me, but I think you should move it to the "Production capacity data sources" section in DATA_SOURCES.md
instead since it's not used by a parser (yet).
What do you think @madsnedergaard?
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.
Moved the URL source to "Production capacity data sources"
biomass, geothermal object removed rather than keeping it zero.
This PR should be mostly good to go (after the clarification on the pdf source), it does however have a formatting conflict with prettier that has to be resolved. I would recommend using the official Prettier extension for vscode but you can also do it via the command line if you wish with If you have any issues let me know and I can help. |
Used prettier format
Moved Tamil Nadu data to "production capacity data sources"
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.
LGTM! 🎉
@gopikrishna1793 Congrats on getting your first PR merged! 🎉 |
Updated installed capacity for India as of May 22.