feat(data-manager): Added a data manager class#81
feat(data-manager): Added a data manager class#81talolard wants to merge 1 commit intomodAL-python:devfrom
Conversation
|
@talolard Hi! I am really sorry, but I forgot to review the PR :( In the last month I was working crazy hard to launch a new startup and a new product, so I set everything aside. If it is still fine by you, I'll review it now and add it. |
cosmic-cortex
left a comment
There was a problem hiding this comment.
I have highlighted some issues which should be changed. Otherwise, I don't have any suggestions to improve the API at the moment, it feels like intuitive for me.
| """ | ||
| This is where the magic happens. | ||
| We take self.unlabeled_mask.nonzero()[0] which gives us an array of the indices that appear in the unlabeled | ||
| data. So if the original label was at position 0 we look up the "real index" in the unlabeled_indices array to | ||
| get it's true index | ||
| :param labels: | ||
| :return: | ||
| """ |
There was a problem hiding this comment.
Can you rewrite the docstrings in google format? (https://www.sphinx-doc.org/en/master/usage/extensions/example_google.html) This is what is used throughout modAL, and it would be best to keep them consistent.
| def _update_masks(self, labels: Union[LabelList, Label]): | ||
| for label in labels: | ||
| self.labeled_mask[label[0]] = True |
There was a problem hiding this comment.
This won't work if labels is a single Label. I suggest to always require a list of labels.
There was a problem hiding this comment.
@talolard Hi! I am really sorry, but I forgot to review the PR :( In the last month I was working crazy hard to launch a new startup and a new product, so I set everything aside. If it is still fine by you, I'll review it now and add it.
Hi, no worries.
I'm moving this into it's own package and will update docs here with an example using it. Hope the launch went well
There was a problem hiding this comment.
Cool! Let me know, when the package is available!
| self._labels[label[0]] = label[1] | ||
|
|
||
| @property | ||
| def unlabeld(self): |
There was a problem hiding this comment.
This is probably a typo, should be unlabeled.
| raise Exception( | ||
| "Malformed input. Please add either a tuple (ix,label) or a list [(ix,label),..]" | ||
| ) |
There was a problem hiding this comment.
This should be a TypeError instead of a generic exception, since it perfectly fits the scope of TypeError.
| correctLabels: LabelList = [] | ||
| unlabeled_indices = self.unlabeled_mask.nonzero()[0] | ||
|
|
||
| for label in labels: | ||
| newIndex = unlabeled_indices[label[0]] | ||
| newLabel: Label = (newIndex, label[1]) | ||
| correctLabels.append(newLabel) |
There was a problem hiding this comment.
Can you rename the variables from camelCase to use lower_and_under conventions?
| for label in labels: | ||
| self.labeled_mask[label[0]] = True | ||
|
|
||
| def _offset_new_labes(self, labels: LabelList): |
There was a problem hiding this comment.
This is just a typo, can you change the function name to _offset_new_labels?
| @property | ||
| def labels(self): | ||
| ''' | ||
|
|
||
| Returns the labels indexed with respect to LD | ||
|
|
||
| ''' | ||
| return self._labels[self.labeled_mask] |
There was a problem hiding this comment.
This can change the data type of labels, if labels_dtype is not explicitly given upon initialization. How about adding a new attribute to the class where we store the label_dtype? This can be None by default and set during the add_labels method, so this method can return self._labels[self.labeled_mask].astype(self.label_dtype). What do you think?
| elif isinstance(labels, list): | ||
| pass | ||
| else: | ||
| raise Exception( | ||
| "Malformed input. Please add either a tuple (ix,label) or a list [(ix,label),..]" | ||
| ) |
There was a problem hiding this comment.
I would write the following here:
elif not isinstance(labels, Container):
raise TypeError("Malformed input. Please add either a tuple (ix,label) or a list [(ix,label),..]")
where Collections is from collections.abc.
|
|
||
| :param features: An array of the features that will be used for AL. | ||
| :param labels: Any prexesiting labels. Each label is a tuple(idx,label) | ||
| :param source: A list of the original data |
There was a problem hiding this comment.
I don't really understand what sources is used for, can you explain it in more detail?
In any case, it is not used by any of the methods (except remaining_sources), so it seems to me that it can be removed.
| for index in range(1): | ||
| # query the learner as usual, in this case we are using a batch learning strategy | ||
| # so indices_to_label is an array | ||
| indices_to_label, query_instance = learner.query(manager.unlabeld) | ||
| labels = [] # Hold a list of the new labels | ||
| for ix in indices_to_label: | ||
| """ | ||
| Here is the tricky part that the manager solves. The indicies are indexed with respect to unlabeled data | ||
| but we want to work with them with respect to the original data. The manager makes this almost transparent | ||
| """ | ||
| # Map the index that is with respect to unlabeled data back to an index with respect to the whole dataset | ||
| original_ix = manager.get_original_index_from_unlabeled_index(ix) | ||
| # print(manager.sources[original_ix]) #Show the original data so we can decide what to label | ||
| # Now we can lookup the label in the original set of labels without any bookkeeping | ||
| y = original_labels_train[original_ix] | ||
| # We create a Label instance, a tuple of index and label | ||
| # The index should be with respect to the unlabeled data, the add_labels function will automatically | ||
| # calculate the offsets | ||
| label = (ix, y) | ||
| # append the labels to a list | ||
| labels.append(label) | ||
| # Insert them all at once. | ||
| manager.add_labels(labels) | ||
| # Note that if you need to add labels with indicies that repsect the original dataset you can do | ||
| # manager.add_labels(labels,offset_to_unlabeled=False) |
There was a problem hiding this comment.
Why do you put this into a for block? the index variable is not used and the iterator which you loop through has a single element.
Codecov Report
@@ Coverage Diff @@
## dev #81 +/- ##
=======================================
Coverage 97.19% 97.19%
=======================================
Files 31 31
Lines 1641 1641
=======================================
Hits 1595 1595
Misses 46 46 Continue to review full report at Codecov.
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #81 +/- ##
=======================================
Coverage 97.19% 97.19%
=======================================
Files 31 31
Lines 1641 1641
=======================================
Hits 1595 1595
Misses 46 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Per #80 made a DataManager class.
Probably easiest to get a sense of it through the examples, but highlight is that users don't need to do bookeeping because they can just do
And the manager keeps everything in sync.
I bet the API can be improved, but it's a solid and convenient start. Looking forward to feedback