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

Heat pump: Tests, Translation, Docstrings, Relocation #89

Merged
merged 5 commits into from
Oct 5, 2024

Conversation

fortrieb
Copy link
Collaborator

@fortrieb fortrieb commented Oct 4, 2024

Heat pump class is refactored for this PR. Following things where done:

  • Translation to english
  • Adding type hints to methods
  • Check inputs for methods
  • Adding Python doc strings to methods
  • Add more units tests to cover all methods
  • Remove class prefix of python file

@michaelosthege
Copy link
Collaborator

Run these to fix code style:

  • pip install -r requirements-dev.txt
  • pre-commit run --all

Copy link
Collaborator

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

Good refactoring!

Mid-term I see the issue of defining heat pumps with different properties, or even multiple heat pumps.
But I think this is a topic that should be covered by a discussion before making implementation changes.

modules/heatpump.py Outdated Show resolved Hide resolved
requirements-dev.txt Outdated Show resolved Hide resolved
Comment on lines 6 to 9
@pytest.fixture(scope="function")
def hp_5kw_24h() -> HeatPump:
"""Heat pump with 5 kw heating output and 24 h prediction"""
return HeatPump(5000, 24)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not put it in test_heatpump to keep the related things together?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thought we can place all fixture in one place. Maybe adding a subfolder fixtures inside tests which can be used for that.

tests/test_heatpump.py Outdated Show resolved Hide resolved
tests/test_heatpump.py Outdated Show resolved Hide resolved
tests/test_heatpump.py Outdated Show resolved Hide resolved
tests/test_heatpump.py Outdated Show resolved Hide resolved
modules/heatpump.py Outdated Show resolved Hide resolved
modules/heatpump.py Outdated Show resolved Hide resolved
@michaelosthege michaelosthege changed the title Heat pump: Tests, Translation, Improments, Relocation Heat pump: Tests, Translation, Docstrings, Relocation Oct 5, 2024
@michaelosthege michaelosthege merged commit 203868c into Akkudoktor-EOS:main Oct 5, 2024
2 checks passed
drbacke pushed a commit that referenced this pull request Oct 6, 2024
* Add first unit test for heatpump COP calculation

* Translate to english,

add type hints, improve unit tests.

* Run pre-commit

* Apply suggestions from code review

Co-authored-by: Michael Osthege <[email protected]>

* Remove conftest file

---------

Co-authored-by: Michael Osthege <[email protected]>
Lasall pushed a commit that referenced this pull request Jan 1, 2025
* Add first unit test for heatpump COP calculation

* Translate to english,

add type hints, improve unit tests.

* Run pre-commit

* Apply suggestions from code review

Co-authored-by: Michael Osthege <[email protected]>

* Remove conftest file

---------

Co-authored-by: Michael Osthege <[email protected]>
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