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.

Sign up for GitHub

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

Jump to bottom

Fix accuracy issues when printing result tables and redesign TableBuilders for more flexibility #404

Closed
struffel opened this issue Nov 17, 2022 · 6 comments · Fixed by #405
Closed

Fix accuracy issues when printing result tables and redesign TableBuilders for more flexibility #404

struffel opened this issue Nov 17, 2022 · 6 comments · Fixed by #405
Assignees
@struffel
Labels
accuracy enhancement

Comments

@struffel
Copy link
Contributor

struffel commented Nov 17, 2022 •
edited by manoelcampos

ISSUE

Actual behavior

The roundTime() function in CloudletsTableBuilder causes incorrect FinishTime display in MinTimeBetweenExample.java

Adjusting the simulation parameters such that VM_MIPS = 100 or CLOUDLET_LENGTH=1000 (basically anything that causes exec time to increase) causes the results table to only apply rounding in some fields. This is particularly noticeable because this example overwrites the default formatting in order to display more decimal digits:

|Cloudlet|Status |DC|Host|Host PEs |VM|VM PEs   |CloudletLen|FinishedLen|CloudletPEs|StartTime|FinishTime|ExecTime
|--------|-------|--|----|---------|--|---------|-----------|-----------|-----------|---------|----------|--------
|      ID|       |ID|  ID|CPU cores|ID|CPU cores|         MI|         MI|  CPU cores|  Seconds|   Seconds| Seconds
|       3|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,17|      2,00|    1,00
|       5|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,64|      1,00|    1,44
|       2|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,70|      1,00|    1,42
|       0|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,87|      1,00|    1,37
|       1|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,91|      1,00|    1,35
|       4|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,96|      1,00|    1,34

Expected behavior

I would expect the table to show increased precision on all fields.

If the issue is related to a specific method, provide a test case that fails in order to show the problem

I was already working in the CloudletsTableBuilder.java file for a future PR so I simply modified the function roundTime() to just return time;, effectively skipping it.
This resolved the issue in this particular example (see output below) but created an inaccuracy in VmDestructionExample (Start = 0, Finish = 15, ExecTime= 14).

New MinTimeBetweenExample.java output (now fixed):

|Cloudlet|Status |DC|Host|Host PEs |VM|VM PEs   |CloudletLen|FinishedLen|CloudletPEs|StartTime|FinishTime|ExecTime
|--------|-------|--|----|---------|--|---------|-----------|-----------|-----------|---------|----------|--------
|      ID|       |ID|  ID|CPU cores|ID|CPU cores|         MI|         MI|  CPU cores|  Seconds|   Seconds| Seconds
|       3|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,17|      1,69|    1,52
|       5|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,64|      2,09|    1,44
|       2|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,70|      2,13|    1,42
|       0|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,87|      2,24|    1,37
|       1|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,91|      2,26|    1,35
|       4|SUCCESS| 1|   0|        4| 0|        4|       1000|       1000|          1|     0,96|      2,30|    1,34

New VmDestructionExample.java (now broken):

                                               SIMULATION RESULTS

|Cloudlet|Status |DC|Host|Host PEs |VM|VM PEs   |CloudletLen|FinishedLen|CloudletPEs|StartTime|FinishTime|ExecTime
|--------|-------|--|----|---------|--|---------|-----------|-----------|-----------|---------|----------|--------
|      ID|       |ID|  ID|CPU cores|ID|CPU cores|         MI|         MI|  CPU cores|  Seconds|   Seconds| Seconds
|       0|SUCCESS| 1|   0|        8| 0|        4|      10000|      10000|          2|        0|        15|      14
|       2|SUCCESS| 1|   0|        8| 0|        4|      10000|      10000|          2|        0|        25|      25

My personal suggestion would be to remove the rounding function and instead display one decimal point of precision for Start/Finish/ExecTime by default and give them an easy way to increase precision even further if needed. This way users see that the times are decimal values and that some rounding errors are to be expected. Otherwise I believe that variations of this rounding issue are going to crop up in other situations again and again.

A pull request for this proposal is in the works.

Specifications like the version of the project, operating system or workload file used

Tested on:

cloudsimplus master 5b8877c
cloudsimplus-examples master aa4b38b
@manoelcampos
Copy link
Collaborator

If the issue is related to a specific method, provide a test case that fails in order to show the problem

I was already working in the CloudletsTableBuilder.java file for a future PR so I simply modified the function roundTime() to just return time;, effectively skipping it. This resolved the issue in this particular example (see output below) but created an inaccuracy in VmDestructionExample (Start = 0, Finish = 15, ExecTime= 14).

Great, but in that case, just remove the roundTime() function instead of making it return the exact value.
Keeping the function will just cause confusion, since it won't do what it's supposed to do.

@manoelcampos
Copy link
Collaborator

New VmDestructionExample.java (now broken):

If we include the decimal places back in that example, it will fix the precision issue.

@manoelcampos
Copy link
Collaborator

manoelcampos commented Nov 17, 2022 •
edited

My personal suggestion would be to remove the rounding function and instead display one decimal point of precision for Start/Finish/ExecTime by default and give them an easy way to increase precision even further if needed.

That can be achieved by changing the constant in CloudletsTableBuilder to an instance field and adding a getter and setter (with 1 decimal place as default).

@struffel
Copy link
Contributor Author

My personal suggestion would be to remove the rounding function and instead display one decimal point of precision for Start/Finish/ExecTime by default and give them an easy way to increase precision even further if needed.

That can be achieved by changing the constant in CloudletsTableBuilder to an instance field and adding a getter and setter (with 1 decimal place as default).

Yes, although the architecture makes this a bit more complicated. Because createTableColumns() gets called during the constructor of the superclass which means that the (non-static) variables of the class are not yet initialized. To solve this I created a small initialization function which ensures that the default values are set at the right moment but there might be a more elegant solution that I am not yet seeing. Here is the current progress: master...StruffelProductions:cloudsimplus:table-time-formatting

While I was at it I also added formatting support for the other fields for things like leading zeros or scientific notation of large numbers but I will explain that in the proper PR. Also planning to submit an example to the example-repo.

@manoelcampos
Copy link
Collaborator

manoelcampos commented Nov 17, 2022 •
edited

Maybe calling createTableColumns() inside the build method fix the issue.
But addColumn() methods should insert the additional fields into a temporary List, adding them to the table only after calling createTableColumns().

@manoelcampos
Copy link
Collaborator

I've just introduced TableColumn.addColumn(columnTitle, columnSubTitle, format) to make it easier to set a format when creating a column.

@struffel struffel mentioned this issue Nov 19, 2022
Merged
@manoelcampos manoelcampos changed the title roundTime() function in CloudletsTableBuilder causes incorrect FinishTime display in MinTimeBetweenExample.java Fix accuracy issues and redesign TableBuilders Nov 21, 2022
@manoelcampos manoelcampos changed the title Fix accuracy issues and redesign TableBuilders Fix accuracy issues when printing result tables and redesign TableBuilders Nov 21, 2022
@manoelcampos manoelcampos changed the title Fix accuracy issues when printing result tables and redesign TableBuilders Fix accuracy issues when printing result tables and redesign TableBuilders for more flexibility Nov 21, 2022
manoelcampos added a commit that referenced this issue Nov 24, 2022
@struffel @manoelcampos
Fix #404 : Redesign TableBuilder and fix printing issues (PR #405)
9232e8e 
* Redesign Table and TableBuilder classes

- Adds new AbstractTableColumn construtor that receives title, subtitle and format
  to make it easier to create columns. Accordingly add such a constructor to subclasses.
- Enables changing format for time, length, ID and PE columns
  in CloudletsTableBuilder.
  This way, the dev can define his/her own default formats upfront.
- Remove the public addColumn methods from Table interface
  and make then protected. Add newColumn methods that just create
  a column, instead of directly adding them to the table.
- When new columns are created, they are added to a temporary list
  before being inserted into the actual table.
  Columns are added to the table just after calling build.
  This way, we can add columns any time and define
  the format for such columns without worring about the order
  of these method calls (as it's expected for a builder class).

* Add setter and getter methods to define formats for CloudletsTableBuilder
* Update CloudletsTableBuilder default precision for time display
* Remove rounding function

* Move createTableColumns() into build method so that columns are just created with the defined format when calling build().
This solves the issue that the variables are not initialized when createTableColumns() is executed.

* Introduces TableBuilderAbstract.addColumn methods for simplicity
* Introduce Table.colCount() for simplicity
* Makes AbstractTableColumn.setTitle to use empty string when null is given.

* Set AbstractTableColumn subtitle default as empty string
* Sets AbstractTableColumn format default as empty string
* Link the column with its table inset the AbstractTable.addColumn()
* Access table attribute directly inside TableBuilderAbstract

* Reinstantiates colsMappings in TableBuilderAbstract instead
of clearing to reduce computational complexity
and avoid concurrency issues.
If column definitions are changed and the build
method is called again, the list of colsMappings
should be cleared before, to add columns
again with new configurations.

* Removed unused methods on TableBuilderAbstract

Signed-off-by: Manoel Campos <manoelcampos@gmail.com>
Co-authored-by: Manoel Campos <manoelcampos@gmail.com>
@manoelcampos manoelcampos mentioned this issue Nov 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@struffel struffel

Labels
accuracy enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add new formatting methods to the TableBuilder and fix printing issues struffel/cloudsimplus
2 participants
@manoelcampos @struffel