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

fixing the negative_samples notebook #1013

Merged
merged 5 commits into from
Jan 9, 2022

Conversation

potipot
Copy link
Contributor

@potipot potipot commented Dec 31, 2021

  • added recordcollection add operator (+)
  • add slicing (slice indexing) for RecordCollection to return a new object of the same class
  • fix the negative samples notebook
  • add tests for the new operator
  • fix failing tests

@potipot potipot marked this pull request as ready for review December 31, 2021 10:49
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@potipot potipot requested a review from FraPochetti December 31, 2021 10:52
@FraPochetti
Copy link
Contributor

A couple of tests are not passing apparently.

@FraPochetti
Copy link
Contributor

why are the changes to RecordCollection needed?

@potipot
Copy link
Contributor Author

potipot commented Dec 31, 2021

These changes allow more intuitive use of the old records list, a bit like regular lists. This slicing and adding is used in the negative samples notebook, but I also came across this use case and already had these commits in my own branch done some time before.

The idea is to be able to

  • do RecordCollection + RecordCollection
  • and make sure that RecordCollection[:n] returns a new RecordCollection instance (and not List[BaseRecord]), cause then we cannot do RecordCollection + RecordCollection[:10] (Operator + not defined for RC and List)

TODO;

You;re right I hurried too much with the PR. I will add tests to the new operator and fix the ones failing.

@potipot potipot marked this pull request as draft December 31, 2021 11:34
@potipot
Copy link
Contributor Author

potipot commented Jan 9, 2022

the failing tests are the known efficientdet metric values

@potipot potipot marked this pull request as ready for review January 9, 2022 17:27
@fstroth
Copy link
Contributor

fstroth commented Jan 9, 2022

Looks good to me. @FraPochetti I think we can merge.

@FraPochetti
Copy link
Contributor

Looks good to me. @FraPochetti I think we can merge.

Yes! Let's merge!

@fstroth fstroth merged commit 5e2543c into airctic:master Jan 9, 2022
@potipot potipot deleted the fix-negative-samples-notebook branch January 10, 2022 11:59
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