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

update PV costs #273

Merged
merged 9 commits into from
Apr 7, 2021
Merged

update PV costs #273

merged 9 commits into from
Apr 7, 2021

Conversation

Piranias
Copy link
Contributor

@Piranias Piranias commented Apr 5, 2021

Fix #xxx

Changes proposed in this pull request:

  • the BOS component costs taken from 2014 are extrapolated for 2019 and 50% higher BOS costs are added because we look at a rooftop system (see working paper section 1a))
  • this results in higher BOS costs
  • the module prices are lowered (after more research). For more information see working paper.
  • the lifetime of PSI is set to 25
  • OM costs are lowered (see working paper)
  • fix number of houses to 20 ( 8 flats per storey makes 40 flats per house with 5 storeys, makes 800 in total (and 480 for 3 storeys))

The following steps were realized, as well (required):

  • Update the CHANGELOG.md
  • Check if full simulation tests pass locally (EXECUTE_TESTS_ON=master pytest)

Also the following steps were realized (if applies):

  • Use in-line comments to explain your code
  • Write docstrings to your code
  • For new functionalities: Explain in readthedocs
  • Write test(s) for your new patch of code

Please mark above checkboxes as following:

  • Open
  • Done

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.

@Piranias Piranias mentioned this pull request Apr 6, 2021
8 tasks
Copy link
Collaborator

@SabineHaas SabineHaas left a 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?

@Piranias
Copy link
Contributor Author

Piranias commented Apr 6, 2021

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)

@Piranias Piranias requested a review from SabineHaas April 6, 2021 14:19
@SabineHaas
Copy link
Collaborator

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:

python-version: [3.7]

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)
Copy link
Collaborator

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.

Copy link
Collaborator

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))

Copy link
Contributor Author

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
Copy link
Collaborator

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.

Copy link
Collaborator

@SabineHaas SabineHaas left a 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 :)

@Piranias Piranias merged commit 8e0bb0a into dev Apr 7, 2021
@Piranias Piranias deleted the fix_PV_costs branch April 7, 2021 07:14
@SabineHaas
Copy link
Collaborator

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:

python-version: [3.7]

For documentation: I had to uncheck a check box in "Branch protection rules" in "Settings/Branches".
From now on 3.6 build should not be triggered anymore @MaGering @Piranias

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants