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

959 Add more tests for various load models Part 1 #1001

Merged
merged 14 commits into from
Aug 31, 2020

Conversation

JacobDomagala
Copy link
Contributor

@JacobDomagala JacobDomagala commented Aug 24, 2020

Part 1

List of Load Models to be tested:

  • LinearModel
  • MultiplePhases
  • NaivePersistence
  • PersistenceMedianLastN

Following will be covered in Part 2:

  • Norm
  • CommOverhead
  • SelectSubphases

@JacobDomagala JacobDomagala changed the title #959 Add unittest for NaivePersistance Load Model 959 Add unittest for NaivePersistance Load Model Aug 24, 2020
@JacobDomagala JacobDomagala changed the title 959 Add unittest for NaivePersistance Load Model 959 Add unittest for NaivePersistence Load Model Aug 24, 2020
@JacobDomagala JacobDomagala force-pushed the 959-add-unit-tests-for-load-models branch 3 times, most recently from 38b2f54 to 3083f9b Compare August 24, 2020 18:20
@codecov
Copy link

codecov bot commented Aug 24, 2020

Codecov Report

Merging #1001 into develop will increase coverage by 0.08%.
The diff coverage is 86.32%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1001      +/-   ##
===========================================
+ Coverage    77.54%   77.62%   +0.08%     
===========================================
  Files          656      666      +10     
  Lines        25390    25554     +164     
===========================================
+ Hits         19688    19836     +148     
- Misses        5702     5718      +16     
Impacted Files Coverage Δ
...nit/collection/test_model_multiple_phases.nompi.cc 83.33% <83.33%> (ø)
...t/collection/test_model_naive_persistence.nompi.cc 85.71% <85.71%> (ø)
...s/unit/collection/test_model_linear_model.nompi.cc 87.09% <87.09%> (ø)
...tion/test_model_persistence_median_last_n.nompi.cc 87.09% <87.09%> (ø)
.../vt/vrt/collection/balance/model/composed_model.cc 88.88% <100.00%> (+11.11%) ⬆️
...lection/balance/model/persistence_median_last_n.cc 94.11% <100.00%> (ø)
...vt/vrt/collection/balance/model/multiple_phases.cc 88.88% <0.00%> (ø)
...llection/balance/model/persistence_median_last_n.h 100.00% <0.00%> (ø)
... and 7 more

@JacobDomagala JacobDomagala force-pushed the 959-add-unit-tests-for-load-models branch 2 times, most recently from 1f59b28 to 0f13dbc Compare August 24, 2020 21:37
@JacobDomagala JacobDomagala force-pushed the 959-add-unit-tests-for-load-models branch 2 times, most recently from d0ed839 to 3ed5cc0 Compare August 25, 2020 13:22
@JacobDomagala JacobDomagala changed the title 959 Add unittest for NaivePersistence Load Model 959 Add more tests for various load models Aug 25, 2020
@JacobDomagala JacobDomagala force-pushed the 959-add-unit-tests-for-load-models branch from a865040 to 8840f33 Compare August 25, 2020 14:45
@JacobDomagala JacobDomagala force-pushed the 959-add-unit-tests-for-load-models branch from 7fa23ec to 0a0ab17 Compare August 25, 2020 19:19
@JacobDomagala JacobDomagala force-pushed the 959-add-unit-tests-for-load-models branch from 0a0ab17 to 423ecc1 Compare August 27, 2020 12:28
@PhilMiller
Copy link
Member

@JacobDomagala could we finish this PR and merge it, and have you open and address issues for the remaining model classes separately? I have some somewhat urgent changes to make on #995 that I'm building on top of this branch.

The only thing I'd ask to have added here before we go ahead with the merging is the fix for the defect you spotted in the Norm model of not actually looping over subphases.

@JacobDomagala JacobDomagala force-pushed the 959-add-unit-tests-for-load-models branch from e6caf84 to 1ca4c6b Compare August 31, 2020 17:51
@JacobDomagala JacobDomagala marked this pull request as ready for review August 31, 2020 17:51
Copy link
Collaborator

Codacy Here is an overview of what got changed by this pull request:

Clones added
============
- tests/unit/collection/test_model_multiple_phases.nompi.cc  6
- tests/unit/collection/test_model_persistence_median_last_n.nompi.cc  8
- tests/unit/collection/test_model_linear_model.nompi.cc  8
- tests/unit/collection/test_model_naive_persistence.nompi.cc  4
         

See the complete overview on Codacy

std::make_pair(TimeType{35}, TimeType{110}), // iter 1 results
std::make_pair(TimeType{60}, TimeType{70}), // iter 2 results
std::make_pair(TimeType{24.5}, TimeType{65}), // iter 3 results
std::make_pair(TimeType{46}, TimeType{-5}), // iter 4 results
Copy link
Member

Choose a reason for hiding this comment

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

This raises an interesting question of whether the linear model should be changed to clamp its result to be >= 0, since negative loads don't really make sense. We can consider that later, though,

Copy link
Collaborator

@lifflander lifflander left a comment

Choose a reason for hiding this comment

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

Looks good to me.

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.

3 participants