E52E Manifold sampling by vivek2000anand · Pull Request #79 · cblearn/cblearn · GitHub
[go: up one dir, main page]
Skip to content

Manifold sampling #79

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
Open
vivek2000anand wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Manifold sampling #79

vivek2000anand wants to merge 10 commits into from

Conversation

@vivek2000anand
Copy link
Contributor
@vivek2000anand vivek2000anand commented May 8, 2024

Hi,

I've added a method to sample points from a line (datasets/_line.py) embedded in a higher dimensional space in the same way that my previous method LinearSubspace did. I've also added a test script (datasets/tests/test_line.py) to unit test its functionality. The code passes all tests and linting. Please do let me know if I need to make any modifications.

Regards
Vivek

@dekuenstle
Copy link
Collaborator
dekuenstle commented May 14, 2024

Hi Vivek,

overall, it's looking good!
Could you please resolve the merge conflicts (i.e., fetch the latest changes from the main to your branch, merge, and push again)?

This is a library for comparison-based data. Would it be helpful to provide utility methods in your manifold classes to sample, e.g., triplets directly? What do you think?

@vivek2000anand
Copy link
Contributor Author
vivek2000anand commented May 17, 2024

Hi David,

Yeah, I'll resolve the merge conflicts. I'm not sure I understand what you mean by adding utility functions to the manifold classes. Could you please elaborate?

@vivek2000anand
Copy link
Contributor Author
vivek2000anand commented Jun 9, 2024

Yup, I've made the changes!

@codecov
Copy link
codecov bot commented Jun 10, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 97.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.88%. Comparing base (3ef9181) to head (51c0703).

Files Patch % Lines
cblearn/datasets/_line.py 95.91% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
+ Coverage   92.62%   92.88%   +0.26%     
==========================================
  Files          44       45       +1     
  Lines        1858     1912      +54     
==========================================
+ Hits         1721     1776      +55     
+ Misses        137      136       -1
Flag Coverage Δ
unittests 92.88% <97.33%> (+0.26%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dekuenstle
Copy link
Collaborator
dekuenstle commented Jun 10, 2024

Sorry, one tiny thing: I just saw, that the abstractmethods are marked "untested". It's fine that they are not tested, but we should remove this mark.

Please:

  1. Replace "pass" by "raise NotImplementedError"
  2. Skip the coverage checks on abstract methods via the .coveragerc: https://stackoverflow.com/a/41284681

@vivek2000anand
Copy link
Contributor Author
vivek2000anand commented Jun 10, 2024

pushed! any other changes needed?

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vivek2000anand @dekuenstle
0