-
Notifications
You must be signed in to change notification settings - Fork 4
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 PV costs #273
update PV costs #273
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.
Shouldn't specific_costs_om
be adapted, as well, in example inputs?
so if I understand correct build 3.6 is deprecated ("Removed python 3.6 from github actions builds as there was a error when installing reverse_geocoder of [report] option of MVS RuntimeError: Python version >= 3.7 required." in PR#271) |
Right, we decided not to test 3.6 anymore. I wonder why it is still triggered.. 3.6 is not stated as version to be tested in the workflow anymore: pvcompare/.github/workflows/main.yml Line 26 in 763df8d
|
CHANGELOG.md
Outdated
@@ -28,7 +28,8 @@ Here is a template for new release sections | |||
### Removed | |||
- | |||
### Fixed | |||
- | |||
- fix PV costs parameters and PSI lifetime (#273) | |||
- fix number of houses to 20 (#273) |
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 think we decided on 30 houses.
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've just seen that you say, there we have 8 flats per storey, not 4. 👍
Could you add the explanation to the changelog please? ( 8 flats per storey makes 40 flats per house with 5 storeys, makes 800 in total (and 480 for 3 storeys))
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.
see the calculation in the description. We assumed a storey had 4 flats, but it's actually 8.
ax.set_xticks(df.step) | ||
ax.set_xlim(ax.get_xlim()[0] - 0.5, ax.get_xlim()[1] + 0.5) | ||
except: | ||
pass |
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.
Did you understand why the plot does not work? If not, please write an issue so that we can fix that later.
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.
So, after adapting changelog.md (more explanation) you can merge :)
For documentation: I had to uncheck a check box in "Branch protection rules" in "Settings/Branches". |
Fix #xxx
Changes proposed in this pull request:
The following steps were realized, as well (required):
EXECUTE_TESTS_ON=master pytest
)Also the following steps were realized (if applies):
Please mark above checkboxes as following:
In case of an error due to linting, run
black . --exclude docs/
and push your changes.Note that in case you do not fix a whole issue you should start your PR with
Address #xyz
.For more information on how to contribute check the CONTRIBUTING.md.