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

[BUG]: unit Test Server Always fails #147

Closed
drbacke opened this issue Oct 9, 2024 · 12 comments
Closed

[BUG]: unit Test Server Always fails #147

drbacke opened this issue Oct 9, 2024 · 12 comments
Labels
bug Something isn't working

Comments

@drbacke
Copy link
Collaborator

drbacke commented Oct 9, 2024

Describe the issue:

BUG]: unit Test Server Always fails

Reproduceable code example:

No response

Error message:

No response

Version information:

@drbacke drbacke added the bug Something isn't working label Oct 9, 2024
@b0661
Copy link
Collaborator

b0661 commented Oct 9, 2024

GET http://127.0.0.1:8503/gesamtlast_simple?year_energy=2000& produces a different result than before.

I do not know whether this is a bug or whether this is acceptable?

If the result of this GET call is not static the test has to be changed. Otherwise there is some difference in the calculation of the result.

Result used to be (and is expected to be):
[
0.12399409965368197,
0.12082955072761034,
0.1186152098599658,
0.13282048719613798,
0.16801614895243458,
0.18833079485066814,
0.2060958576163904,
0.23815979421159755,
0.256794470747979,
0.2730286944402595,
0.2837831370699189,
0.2880791113592318,
0.27868845536305187,
0.2593976275300202,
0.26565341182134644,
0.2834322084492842,
0.3385133447353511,
0.36541124595269503,
0.33781770981176945,
0.29218838246014817,
0.24535542466592053,
0.19484917288665324,
0.15634028941335018,
0.13309410380383394,
0.1239736903705896,
0.12074280282759843,
0.11849705069574769,
0.1320224904313182,
0.16699106562771282,
0.18856343364342354,
0.20578134176859084,
0.23760516695843145,
0.25675411140321175,
0.2729442256919556,
0.2837782018548114,
0.2882881388086184,
0.2800061651414149,
0.26039842637914645,
0.26613497240454587,
0.28441116400389416,
0.33941195977626437,
0.36621100374643395,
0.3391062422788601,
0.29391186081171267,
0.24686897327498972,
0.19620626090788126,
0.1570820352949701,
0.13332596485058107
]

Result now is
[
0.1239736903705896,
0.12074280282759843,
0.11849705069574769,
0.1320224904313182,
0.16699106562771282,
0.18856343364342354,
0.20578134176859084,
0.23760516695843145,
0.25675411140321175,
0.2729442256919556,
0.2837782018548114,
0.2882881388086184,
0.2800061651414149,
0.26039842637914645,
0.26613497240454587,
0.28441116400389416,
0.33941195977626437,
0.36621100374643395,
0.3391062422788601,
0.29391186081171267,
0.24686897327498972,
0.19620626090788126,
0.1570820352949701,
0.13332596485058107,
0.12395328108749722,
0.1206560549275865,
0.11837889153152954,
0.1312244936664984,
0.16596598230299103,
0.18879607243617894,
0.20546682592079127,
0.2370505397052653,
0.2567137520584444,
0.2728597569436517,
0.2837732666397039,
0.2884971662580049,
0.281323874919778,
0.2613992252282727,
0.2666165329877454,
0.28539011955850424,
0.34031057481717775,
0.367010761540173,
0.3403947747459508,
0.2956353391632773,
0.24838252188405893,
0.1975633489291094,
0.15782378117659007,
0.13355782589732823
]

@NormannK
Copy link
Collaborator

NormannK commented Oct 9, 2024

reason is still PR #135 which was added without all tests passing.

@drbacke
Copy link
Collaborator Author

drbacke commented Oct 9, 2024

reason is still PR #135 which was added without all tests passing.

Yes that was too early. Revert?
@NormannK Short question: You wanted to be a maintainer? Would be very sensefull in my opionion. You are the Most active Person Here👍

Lasall added a commit to Lasall/EOS that referenced this issue Oct 9, 2024
 * Don´t rely on calculation per specific day. For now we can assume 48
   values (2 days prediction).
@NormannK
Copy link
Collaborator

NormannK commented Oct 9, 2024

I just checked the results vs a old version (03.10.) and I get the new values.

0 | 0.1239736903705896
1 | 0.12074280282759843
2 | 0.11849705069574769
3 | 0.1320224904313182
4 | 0.16699106562771282
5 | 0.18856343364342354
6 | 0.20578134176859084
7 | 0.23760516695843145
8 | 0.25675411140321175
[...]

@drbacke I'm sorry I'd like to contribute but id like to stick to what I can. And I'm not good at git stuff.
But feel free to give me something specific todo.

Lasall added a commit to Lasall/EOS that referenced this issue Oct 9, 2024
 * Don´t rely on calculation per specific day. For now we can assume 48
   values (2 days prediction).
Lasall added a commit to Lasall/EOS that referenced this issue Oct 9, 2024
 * Don´t rely on calculation per specific day, just verify length.
Lasall added a commit to Lasall/EOS that referenced this issue Oct 9, 2024
 * Don´t rely on calculation per specific day, just verify length.
@b0661
Copy link
Collaborator

b0661 commented Oct 9, 2024

@Lasall proposes #148 to just check for the number of values in the result.

For the devs: is one of the datasets the right one (and we can check for it) or do we have to reduce the sensitivity of the test?

@drbacke
Copy link
Collaborator Author

drbacke commented Oct 9, 2024

I think this is due to numeric reasons. Multiply the year_energy by 1000 and the results will be more stable. Its normailized in Wh.
Wh ist numerical very sensefull, because WE are in a 1h system and so Watt and Wh are equal. This saves time
And use a small numerical tolerance (about 1e-5 )

@b0661
Copy link
Collaborator

b0661 commented Oct 9, 2024

reason is still PR #135 which was added without all tests passing.

PR #125 which activated server testing by #135 was tested by me on the 8th of October. There it worked. Any commit in here 565e721, 9e3dd27, 2882ca4, 6b545c5, d804f5d, 929d7e0, b6d0ef2, 3c1482c changed the behaviour.

Edit: There seems to be a date/time dependency in the calculation, so my blame on the above commits is wrong.

@Lasall
Copy link
Collaborator

Lasall commented Oct 9, 2024

I don´t think there is a numerical issue here, just one day offset (the overlapping calculated numbers are exactly even). /gesamtlast_simple has some forecast based on start date (date.now) + 48 hours. So either we fixate the requested interval or we don´t care about numbers. Imho an integration test should just check the correct output format while the unit tests have to verify calculations.

@b0661
Copy link
Collaborator

b0661 commented Oct 9, 2024

Imho an integration test should just check the correct output format while the unit tests have to verify calculations.

I agree, take server tests as integration tests and go for the number of values only.

@drbacke
Copy link
Collaborator Author

drbacke commented Oct 9, 2024

I don´t think there is a numerical issue here, just one day offset (the overlapping calculated numbers are exactly even). /gesamtlast_simple has some forecast based on start date (date.now) + 48 hours. So either we fixate the requested interval or we don´t care about numbers. Imho an integration test should just check the correct output format while the unit tests have to verify calculations.

  1. 4000 Wh per year will result in a numerical issue. Factor 1000 wrong, this will result in a Higher numerical Error due to the normalization.
  2. You need to use the same Date, then you will get the same result. The load Profile Changes every day.
  3. I wrote the function and created the Data, so believe me 😊

@b0661
Copy link
Collaborator

b0661 commented Oct 9, 2024

I don´t think there is a numerical issue here, just one day offset (the overlapping calculated numbers are exactly even). /gesamtlast_simple has some forecast based on start date (date.now) + 48 hours. So either we fixate the requested interval or we don´t care about numbers. Imho an integration test should just check the correct output format while the unit tests have to verify calculations.

1. 4000 Wh per year will result in a numerical issue. Factor 1000 wrong, this will result in a Higher numerical Error due to the normalization.

2. You need to use the same Date, then you will get the same result. The load Profile Changes every day.

3. I wrote the function and created the Data, so believe me 😊

My conclusion:

  • Use the simple test case Fix test_server Closes #147 #148 proposed by @Lasall for server testing. In server testing no code coverage can be analysed. It is really all about the interface.
  • Add an unit test that tests the calculation. Unit testing allows for code coverage and can be a lot more specific to the functions.

@NormannK
Copy link
Collaborator

NormannK commented Oct 9, 2024

I agree with @b0661

drbacke pushed a commit that referenced this issue Jan 1, 2025
 * Don´t rely on calculation per specific day, just verify length.
Lasall added a commit that referenced this issue Jan 1, 2025
 * Don´t rely on calculation per specific day, just verify length.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants