Repository navigation
Conversation
janvanrijn
left a comment
janvanrijn
left a comment
There was a problem hiding this comment.
Seems like the substantial part of this PR are many dicts to OrderedDicts, upgrade of liac-arff and more strict unit tests
| run.trace_attributes = trace['attributes'] | ||
| run.trace_content = trace['data'] | ||
| data_tmp = trace['data'] | ||
| # TODO move this functionality to trace.py and serialize/deserialize the trace |
There was a problem hiding this comment.
Seems like an unresolved TODO. Is there a reason to not do this immediately?
There was a problem hiding this comment.
Yes, I currently don't have the time. This is related to #482 and should be resolved in there.
There was a problem hiding this comment.
Ok. can you open an issue for this?
I don't get why you need to cast the first three items to an int, and the fourth to a float. Aren't the data types maintained by the arff dumps function?
There was a problem hiding this comment.
I created issue #483. The casts are necessary because arff only defines values to be numerical, not float or int. The cast to float isn't there any more, right?
There was a problem hiding this comment.
I assume not, as all other columns should be float as well (the predictions per class and the 'predicted' and 'correct' class)
The thing I am mostly worried about is the 'assumptions' in the code, i.e., if we ever add one column we don't have the overview where these kind of manual patches are. Therefore I would propose to move this part to the trace file anyway (I can do this later today, if that helps)
There was a problem hiding this comment.
They are all represented as float, regardless whether they are floats or ints. I totally agree, if you have the time to implement #483 this would definitively be great.
There was a problem hiding this comment.
No problem. In that case I guess you would have to also review this PR.
There was a problem hiding this comment.
I moved the functionality to the trace.py. While doing so, I realized that the current way of representing the trace (in arff) is quite hacky, and we should use the actual trace object throughout the run. This update will make things way more neat, but it will cost several hours to implement, which I can't do / test right now.
I think the current solution moves the functionality to exactly where one would expect it to be and is fine for now. Furthermore, I made it a private function, so we don't have a real obligation to maintain it in future versions.
|
The most important part is the new unit tests. All other changes are fixes to make the unit test pass. |
Codecov Report
@@ Coverage Diff @@
## develop #481 +/- ##
===========================================
- Coverage 89.59% 89.54% -0.05%
===========================================
Files 32 32
Lines 2777 2803 +26
===========================================
+ Hits 2488 2510 +22
- Misses 289 293 +4
Continue to review full report at Codecov.
|
That's why I shied away from this in the first place. Besides that, thanks for your changes, they look great! |
|
Let's merge then |
3 participants
This PR adds a unit test for #422 which will fail with the current liac-arff release 2.2.1. Upgrading to liac-arff 2.2.2 will make the unit test work again (release is under preparation).