Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

feat(data-manager): Added a data manager class #81

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

Open
talolard wants to merge 1 commit into modAL-python:dev
base: dev
Choose a base branch
Loading
from talolard:Feature/data-manager

Conversation

@talolard
Copy link

@talolard talolard commented Apr 3, 2020

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

 for ix in indices_to_label:
 original_ix = manager.get_original_index_from_unlabeled_index(ix)
 y = original_labels_train[original_ix]
 label = (ix, y)
 labels.append(label)
 manager.add_labels(labels)
 # clear_output()
 learner.teach(manager.labeled, manager.labels)

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

Copy link
Member

@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.

Copy link
Member

@cosmic-cortex cosmic-cortex left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +62 to +69
"""
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:
"""
Copy link
Member

@cosmic-cortex cosmic-cortex May 9, 2020

Choose a reason for hiding this comment

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

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.

Comment on lines +57 to +59
def _update_masks(self, labels: Union[LabelList, Label]):
for label in labels:
self.labeled_mask[label[0]] = True
Copy link
Member

@cosmic-cortex cosmic-cortex May 9, 2020

Choose a reason for hiding this comment

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

This won't work if labels is a single Label. I suggest to always require a list of labels.

Copy link
Author

Choose a reason for hiding this comment

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

@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

Copy link
Member

@cosmic-cortex cosmic-cortex May 14, 2020

Choose a reason for hiding this comment

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

Cool! Let me know, when the package is available!

self._labels[label[0]] = label[1]

@property
def unlabeld(self):
Copy link
Member

@cosmic-cortex cosmic-cortex May 9, 2020

Choose a reason for hiding this comment

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

This is probably a typo, should be unlabeled.

Comment on lines +88 to +90
raise Exception(
"Malformed input. Please add either a tuple (ix,label) or a list [(ix,label),..]"
)
Copy link
Member

@cosmic-cortex cosmic-cortex May 9, 2020

Choose a reason for hiding this comment

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

This should be a TypeError instead of a generic exception, since it perfectly fits the scope of TypeError.

Comment on lines +73 to +79
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)
Copy link
Member

@cosmic-cortex cosmic-cortex May 9, 2020

Choose a reason for hiding this comment

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

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):
Copy link
Member

@cosmic-cortex cosmic-cortex May 9, 2020

Choose a reason for hiding this comment

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

This is just a typo, can you change the function name to _offset_new_labels?

Comment on lines +39 to +46
@property
def labels(self):
'''

Returns the labels indexed with respect to LD

'''
return self._labels[self.labeled_mask]
Copy link
Member

@cosmic-cortex cosmic-cortex May 9, 2020

Choose a reason for hiding this comment

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

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?

Comment on lines +85 to +90
elif isinstance(labels, list):
pass
else:
raise Exception(
"Malformed input. Please add either a tuple (ix,label) or a list [(ix,label),..]"
)
Copy link
Member

@cosmic-cortex cosmic-cortex May 9, 2020

Choose a reason for hiding this comment

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

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
Copy link
Member

@cosmic-cortex cosmic-cortex May 9, 2020

Choose a reason for hiding this comment

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

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.

Comment on lines +101 to +125
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)
Copy link
Member

@cosmic-cortex cosmic-cortex May 9, 2020

Choose a reason for hiding this comment

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

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.

Copy link

Codecov Report

Merging #81 (3281990) into dev (1e23453) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph 

@@ 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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e23453...3281990. Read the comment docs.

Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.19%. Comparing base (1e23453) to head (3281990).
Report is 115 commits behind head on dev.

❗ 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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cosmic-cortex cosmic-cortex cosmic-cortex left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /