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

added get_cop to heat_pump.py #7

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

added get_cop to heat_pump.py #7

wants to merge 6 commits into from

Conversation

Pyosch
Copy link

@Pyosch Pyosch commented Nov 20, 2018

Initial discussion started here
Since the function is not in the HeatBuilding class in demandlib.bdew anymore I changed the parameter self to heatbuilding.

References
----------
.. [1]: 'https://www.researchgate.net/publication/255759857_A_review_of_domestic_heat_pumps'
Research paper about domestic heatpumps, containing the formulas used
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add:
Returns

@jnnr
Copy link
Member

jnnr commented Jul 25, 2019

Thanks for your effort! And sorry to see that this is quite a late reply. If this PR is still active, please do the following:

  • Change base to dev.
  • Place the module heat_pump.py in the new directory structure: src/oemof/thermal/heat_pump.py

@Pyosch Pyosch changed the base branch from master to dev September 16, 2019 12:16
@Pyosch
Copy link
Author

Pyosch commented Sep 16, 2019

Thank you for your feedback! I hope it works for you now.

@uvchik
Copy link
Member

uvchik commented Oct 1, 2019

Thank you for your approach.

I do not know if it is a good idea to create a dependency on the demandlib. This will make it more difficult to use it for other purposes.

def get_cop(building_temperature, heatpump_type="Air", water_temp=60):
...
return cop

If you want to use it together with the demandlib you can just use the following code:

heatbuilding.cop = get_cop(heatbuilding.temperature)

For everybody else it would be much easier because users will not have to install the demandlib and instantiate a heat_building they do not want use just to pass a temperature time series.

@uvchik
Copy link
Member

uvchik commented Oct 2, 2019

@Pyosch We are planning to use the MIT licence in the future. Would you agree that your contribution will be under this licence? We need your agreement for the demandlib as well but you can just write a comment here for both contributions. Thanks in advance.

@Pyosch
Copy link
Author

Pyosch commented Oct 2, 2019

@uvchik yes I don't mind. See you in Berlin ;)

@uvchik uvchik requested a review from jnnr October 5, 2019 09:09
src/oemof/thermal/heat_pump.py Show resolved Hide resolved
@jnnr jnnr requested a review from jakob-wo October 5, 2019 09:43
@uvchik uvchik mentioned this pull request Oct 5, 2019
@jnnr
Copy link
Member

jnnr commented Oct 5, 2019

@jakob-wo, you have been looking into the heat pump topic recently. I guess your feedback would be really helpful here!

Copy link
Member

@jnnr jnnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your effort! An example that shows the new function would be nice (in oemof-thermal/examples). You could get in contact with @jakob-wo, who is developing functions for heat pumps on a branch at the moment (not yet a PR: https://github.com/oemof/oemof-thermal/tree/features/cmpr_heatpumps_and_chillers/examples)

@jakob-wo
Copy link
Contributor

jakob-wo commented Oct 7, 2019

Hi @Pyosch,

thanks for enriching the COP calculation with field test data - I like that approach.

As mentioned already above I am also about to come up with a COP calculation for compression heat pumps. I am planning to call the module src/compression_heatpumps_and_chillers.py (features/cmpr_heatpumps_and_chillers).
Furthermore, there is going to be a module on absorptions heat pumps somewhen in the near future.

It would be good to name your module clearly and distinguish between compression and absorption. I also think it might be helpful to make clear, that the module contains hard coded parameters/coefficients by including somethink like "field_test_regression" in the name.

@jakob-wo
Copy link
Contributor

jakob-wo commented Oct 7, 2019

One more remark:
The paper gives a temperature range the equations (Eq. 4) can be applied in.
Wouldn't it be nice to check whether the input data fullfills that condition and raise a UserWarning if someone exceeds the limit?

@Pyosch
Copy link
Author

Pyosch commented Oct 8, 2019

@uvchik, @jnnr and @jakob-wo thank you for your feedback! I'll look into it again. @jakob-wo your approach sounds very interesting too. I am looking forward to try it out.

@jakob-wo jakob-wo mentioned this pull request Nov 6, 2019
2 tasks
@jnnr
Copy link
Member

jnnr commented Nov 6, 2019

Seems like travis does not run the tests properly. Might be because this PR comes from a fork. Anybody got an idea how to fix this?

@jnnr jnnr mentioned this pull request Nov 14, 2019
@Pyosch
Copy link
Author

Pyosch commented Dec 6, 2019

I added the warning messages and an example. The import of heat_pump inside the example is not correct I guess, but I couldn't quite figure out how the import is supposed to be structured. I looked at the other examples but it didn't help.

Any advice is appreciated :)

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.

4 participants