-
Notifications
You must be signed in to change notification settings - Fork 17
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
Principal Curves #338
Principal Curves #338
Conversation
🔨 Explore the source changes: e67f279 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff.
Please add tests here and make sure the PR checks pass.
self.s_factor = s_factor | ||
|
||
def project(self, X, p, s): | ||
''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please redo docs with Google documentation style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs changed, satisfactory?
#Copyright 2020, zsteve | ||
#Written entirely by https://github.com/zsteve | ||
#https://github.com/zsteve/pcurvepy | ||
#slightly modified by https://github.com/CaseyWeiner, 2022 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add sentence describing how you modified it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, sufficient?
Test added, lmk if you want more |
@tathey1 Check fails due to |
Hmm, maybe the AWS permissions don't work on forks... I'll try to test that. In the meantime, some conflicts arose from my recent PR adding type hints (e.g. my new changes to state_generation). Please add these to your code too (shouldn't take long, and I think this will be the new standard for the package. |
Merged your typing changes into fork. Checks fail for same reason, but rest is good to go |
great, last thing can you add the type hints to the code you added? |
@tathey1 Do you mean me merging your type hints, or me adding type hints for the function I wrote, |
@CaseyWeiner please add type hints to all the code you are contributing. |
Also, i believe the check error is due to this: #343. So for now, you're ok but in the future, let's do all your development from a branch in brainlit, rather than a fork. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
@CaseyWeiner thanks for your patience on this. Can you merge with my new changes on |
I think you solved it @tathey1 ! The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, although i think pcurve could be called _pcurve.py
since it is for internal use.
Addresses part of #321
_pc_endpoints_from_coords_neighbors
inbrainlit/algorithms/generate_fragments/state_generation.py
that estimates endpoints using Principal Curves (Hastie '89).