-
Notifications
You must be signed in to change notification settings - Fork 151
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
create align module #410
create align module #410
Conversation
Deploy preview for graspy ready! Built with commit 6cb745d |
Codecov Report
@@ Coverage Diff @@
## master #410 +/- ##
==========================================
- Coverage 88.10% 87.88% -0.22%
==========================================
Files 41 46 +5
Lines 2698 3014 +316
==========================================
+ Hits 2377 2649 +272
- Misses 321 365 +44
Continue to review full report at Codecov.
|
…m, overloaded fit_transform in sign flips
52784a9
to
6cb745d
Compare
here is exact log of me trying to rebase dis.
and i end up with a diff that contains commits already on the master, which i just reverted, as u see above. |
I'm having a hard time following everything. Do you have a "clean" version of this branch anywhere, that just has your changes, and no rebases/merges? |
it looks like maybe that is this branch? |
Given your message, it might be bc you didn't pull master before rebasing? BUT, Im just not sure what the problem you were trying to solve by rebasing? are you rebasing because master is ahead of your First you dont have to rebase just because master is ahead. Its only necessary if you need current changes reflected in your branch or if there were changes in master that prevents you from merging due to conflicts. Second, rebasing, in my opinion, is a pain and merging is just easier. The commits within a PR gets squashed when merged to master, so the commit history in that PR doesn't matter at all. Thats why I just merge instead of rebase. |
this is the one. it has one revert, but i believe that since i did it in a hacky way - it doesn't show up in the commit history, only on github. |
unfortunately did check that by pulling twice while being specifically on the master before doing anythign else, so this can't be the issue.
that is true but see below...
agreed (but still see below).
correct, but: i will likely cause a conflict if i do not rebase if i do further work, because i was going to at least remove the sign flips function from the ldt and substitute it with the object from the align module, and ideally i'd want to add in seedless procrustes as well. since the ldt has been modified heavily since the time i branched, it will very likely cause conflicts and me no wants to be resolving them post-factum. unless i'll be looking at the diff from that pr while working and be very careful in not touching the same lines, but this feels like a lot of effort that can be avoided if i just rebase. in short, i'm not after dcb7047, but after #411 changes when i'm trying to rebase. alternatively i could make using align in other modules a part of a different pr.
i wouldn't mind doing this, but there was something weird about the merge from master to a side branch that didn't show up right when i did that with, i think, #336 , so i decided to start doing rebases instead. |
moving onto #419 since that rebase worked out correctly. will leave this open 4 temporarily. |
Reference Issues/PRs
Fixes #367
What does this implement/fix? Explain your changes.
WIP
Any other comments?
WIP