Skip to content

Conversation

avgprog
Copy link
Contributor

@avgprog avgprog commented Jan 21, 2021

Resolves #236. Changes done -

  1. Created a class SystemParticipantWithHeatResult to include the additional thermal power variable qDot.
  2. Extended ChpResult.java to inherit this class.
  3. Modified SystemParticipantResultFactory.java to add the additional thermal power variable.

Several test cases are failing now. I am trying to resolve the issues.

@avgprog
Copy link
Contributor Author

avgprog commented Jan 24, 2021

I have modified the test cases according to the new code. But, there are some codacy static code analysis issues which I have fixed but they are being shown. And there are several test cases which are failing.

@avgprog
Copy link
Contributor Author

avgprog commented Jan 27, 2021

Regarding the test cases which are failing, I tried running ./gradlew test on the dev branch, the 5 test cases which fail on my branch sb/#236-modifyingChpResult are also failing on the dev branch. The Codacy Static Code Analysis test is successful now. I made a mistake while understanding the error messages before.

@avgprog avgprog changed the title #236 Added SystemParticipantWithHeatResult class and modified ChpResult accordingly Added SystemParticipantWithHeatResult class and modified ChpResult accordingly Jan 28, 2021
@ckittl
Copy link
Member

ckittl commented Jan 28, 2021

!test

1 similar comment
@ckittl
Copy link
Member

ckittl commented Jan 29, 2021

!test

@avgprog
Copy link
Contributor Author

avgprog commented Jan 29, 2021

Need to add more test cases to test SystemParticipantWithHeatResult class probably to increase the code coverage.

@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #269 (492ae15) into dev (01ba298) will decrease coverage by 0.24%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #269      +/-   ##
============================================
- Coverage     78.31%   78.06%   -0.25%     
- Complexity     1941     1944       +3     
============================================
  Files           246      247       +1     
  Lines          7885     7859      -26     
  Branches        746      748       +2     
============================================
- Hits           6175     6135      -40     
- Misses         1293     1307      +14     
  Partials        417      417              
Impacted Files Coverage Δ Complexity Δ
...edu/ie3/datamodel/exceptions/FactoryException.java 33.33% <0.00%> (-33.34%) 1.00% <0.00%> (-1.00%)
...io/factory/input/participant/EvcsInputFactory.java 66.66% <0.00%> (-33.34%) 3.00% <0.00%> (ø%)
...atamodel/models/input/thermal/ThermalBusInput.java 66.66% <0.00%> (-22.23%) 2.00% <0.00%> (-1.00%)
...amodel/models/input/thermal/ThermalHouseInput.java 54.16% <0.00%> (-22.15%) 5.00% <0.00%> (-1.00%)
...tamodel/models/input/thermal/ThermalUnitInput.java 42.10% <0.00%> (-13.90%) 4.00% <0.00%> (ø%)
.../models/input/thermal/CylindricalStorageInput.java 73.68% <0.00%> (-11.57%) 11.00% <0.00%> (ø%)
...ain/java/edu/ie3/datamodel/io/factory/Factory.java 80.00% <0.00%> (-8.00%) 14.00% <0.00%> (ø%)
.../ie3/datamodel/models/result/system/ChpResult.java 100.00% <0.00%> (ø) 3.00% <0.00%> (ø%)
...result/system/SystemParticipantWithHeatResult.java 100.00% <0.00%> (ø) 4.00% <0.00%> (?%)
...factory/result/SystemParticipantResultFactory.java 95.45% <0.00%> (+0.14%) 26.00% <0.00%> (+1.00%)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 375a2dd...3b16fb8. Read the comment docs.

@avgprog avgprog marked this pull request as ready for review February 3, 2021 07:52
Copy link
Member

@ckittl ckittl left a comment

Choose a reason for hiding this comment

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

Thank you for your work on this. Please find some remarks below.

Additionally, please let HpResult also be a child of SystemParticipantWithHeatResult and remove the qDot-attribute there. Finally, please adapt the uml documentation for result classes (OutputDataModelConcept.puml) as well as the corresponding section in Sphinx-Documentation (docs/readthedocs/models/result/participant/chp.rst)

@avgprog
Copy link
Contributor Author

avgprog commented Feb 5, 2021

Actually, I have made the suggested changes (except for the changes to the uml and Sphinx documentation) to the pull request 269 but the problem is that there is Quality Gate failure. I believe the problem is that the added functions in the class SystemParticipantWithHeatResult.java have not been tested.

@avgprog avgprog requested a review from ckittl February 11, 2021 10:32
ckittl and others added 3 commits February 11, 2021 12:42
@avgprog avgprog requested a review from ckittl February 15, 2021 12:52
@avgprog
Copy link
Contributor Author

avgprog commented Feb 15, 2021

I have added the requested changes to the puml file and the SystemParticipantResultFactory class. I also modified AUTHORS and CHANGELOG.md. However, when I was making the first two changes, I realized that in OutputDataModelConcept.puml, StorageResult and EvResult are defined in the same way I did previously for ChpResult and HpResult. So, I believe we need to change that. Also, a similar change to what you suggested for the if condition containing ChpResult and HpResult in SystemParticipantResultFactory class can be applied for the if condition containing StorageResult and EvResult.

Right now, several test cases are failing here but they all seem to pass on my local machine.

@ckittl
Copy link
Member

ckittl commented Feb 17, 2021

I have added the requested changes to the puml file and the SystemParticipantResultFactory class. I also modified AUTHORS and CHANGELOG.md. However, when I was making the first two changes, I realized that in OutputDataModelConcept.puml, StorageResult and EvResult are defined in the same way I did previously for ChpResult and HpResult. So, I believe we need to change that. Also, a similar change to what you suggested for the if condition containing ChpResult and HpResult in SystemParticipantResultFactory class can be applied for the if condition containing StorageResult and EvResult.

Right now, several test cases are failing here but they all seem to pass on my local machine.

Thanks for that catch! I would suggest to only stick to one issue here (this is adding the heat part). This makes it easier to keep track of PRs.

@ckittl
Copy link
Member

ckittl commented Feb 17, 2021

!test

ckittl
ckittl previously approved these changes Feb 17, 2021
Copy link
Member

@ckittl ckittl left a comment

Choose a reason for hiding this comment

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

Please wait with merging, until the tests are passing.

@ckittl ckittl merged commit 45235fe into dev Feb 17, 2021
@ckittl ckittl deleted the sb/#236-modifyingChpResult branch February 17, 2021 11:17
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.

Refactor ChpResult
2 participants