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

create align module #410

Closed
wants to merge 21 commits into from
Closed

create align module #410

wants to merge 21 commits into from

Conversation

alyakin314
Copy link
Contributor

Reference Issues/PRs

Fixes #367

What does this implement/fix? Explain your changes.

WIP

Any other comments?

WIP

@netlify
Copy link

netlify bot commented Aug 31, 2020

Deploy preview for graspy ready!

Built with commit 6cb745d

https://deploy-preview-410--graspy.netlify.app

@codecov
Copy link

codecov bot commented Aug 31, 2020

Codecov Report

Merging #410 into master will decrease coverage by 0.21%.
The diff coverage is 79.56%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
graspy/cluster/autogmm.py 91.76% <ø> (ø)
graspy/cluster/kclust.py 100.00% <ø> (ø)
graspy/datasets/base.py 100.00% <ø> (ø)
graspy/embed/ase.py 100.00% <ø> (ø)
graspy/embed/base.py 94.11% <ø> (ø)
graspy/embed/lse.py 100.00% <ø> (ø)
graspy/embed/mase.py 95.45% <ø> (ø)
graspy/embed/mds.py 100.00% <ø> (ø)
graspy/embed/omni.py 96.55% <ø> (ø)
graspy/embed/svd.py 100.00% <ø> (ø)
... and 25 more

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 dcb7047...6cb745d. Read the comment docs.

@alyakin314
Copy link
Contributor Author

@bdpedigo @j1c
not a major issue, but just pointing out that here there were no rebases and github diff shows up fine, but codev still shows many files in diff that shouldnt be there. not sure what's up with that commit/pr that it acts so weirdly everywhere else.

@alyakin314 alyakin314 changed the title seedless procrustes added as a part of the align module create align module Aug 31, 2020
@alyakin314
Copy link
Contributor Author

@bdpedigo, @j1c

here is exact log of me trying to rebase dis.

(graphs) antonalyakin@Antons-MacBook-Pro graspy % git checkout align-module
Switched to branch 'align-module'
Your branch is up to date with 'origin/align-module'.
(graphs) antonalyakin@Antons-MacBook-Pro graspy % git rebase master
First, rewinding head to replay your work on top of it...
Applying: seedless procrustes added as a part of the align module
Applying: created base aligner
Applying: seedless procrustes and sign flips modified such that they work with BaseAlign
Applying: sign flips modified
Applying: preliminary sign flips and seedless procrustes done; sign flips works as intended
Applying: wrote many sign flips test, slightly changed the behavior of transform, overloaded fit_transform in sign flips
Applying: sign flips done, with tests yayayay
Applying: forgot to add a test for max criteria; also fixed a bug
Applying: test sign flips changed to have 100% coverage
Applying: created tests for seedless procrustes (unpopulated yet)
Applying: created orthogonal procrustes file
Applying: sign flips and orthogonal procrustes now have checks and doc strings
Applying: blacked files
Applying: created tests for orthogonal procrustes
Applying: init bug
Applying: orthogonal procrustes passes corresponding tests
Applying: black orthogonal procrustes tests file
Applying: massively simplified seedless procrustes code; speeded it up by no longer doing 100 iterations"
Applying: added ot (optimal transport) to requirements
Applying: wrong name of the package
Applying: set-up tiny fix
(graphs) antonalyakin@Antons-MacBook-Pro graspy % git rebase master
Current branch align-module is up to date.
(graphs) antonalyakin@Antons-MacBook-Pro graspy % git push
To https://github.com/neurodata/graspy.git
 ! [rejected]        align-module -> align-module (non-fast-forward)
error: failed to push some refs to 'https://github.com/neurodata/graspy.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
(graphs) antonalyakin@Antons-MacBook-Pro graspy % git pull
Merge made by the 'recursive' strategy.
(graphs) antonalyakin@Antons-MacBook-Pro graspy % git push
Enumerating objects: 127, done.
Counting objects: 100% (127/127), done.
Delta compression using up to 8 threads
Compressing objects: 100% (110/110), done.
Writing objects: 100% (110/110), 13.52 KiB | 2.70 MiB/s, done.
Total 110 (delta 79), reused 1 (delta 0)
remote: Resolving deltas: 100% (79/79), completed with 13 local objects.
To https://github.com/neurodata/graspy.git
   6cb745d..52784a9  align-module -> align-module

and i end up with a diff that contains commits already on the master, which i just reverted, as u see above.

@alyakin314
Copy link
Contributor Author

see #418 for my first attempt trying to rebase to master, and #419 for my second attempt.
i then tried to recreate here exactly what i did in #419, but github gods did not favor me.

@bdpedigo
Copy link
Collaborator

bdpedigo commented Sep 3, 2020

see #418 for my first attempt trying to rebase to master, and #419 for my second attempt.
i then tried to recreate here exactly what i did in #419, but github gods did not favor me.

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?

@bdpedigo
Copy link
Collaborator

bdpedigo commented Sep 3, 2020

it looks like maybe that is this branch?

@j1c
Copy link
Collaborator

j1c commented Sep 3, 2020

see #418 for my first attempt trying to rebase to master, and #419 for my second attempt.
i then tried to recreate here exactly what i did in #419, but github gods did not favor me.

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 align-module branch? As far as I can see, you started the branch with dcb7047.. or is that because you rebased?

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.

@alyakin314
Copy link
Contributor Author

it looks like maybe that is this branch?

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.

@alyakin314
Copy link
Contributor Author

see #418 for my first attempt trying to rebase to master, and #419 for my second attempt.
i then tried to recreate here exactly what i did in #419, but github gods did not favor me.

Given your message, it might be bc you didn't pull master before rebasing?

unfortunately did check that by pulling twice while being specifically on the master before doing anythign else, so this can't be the issue.

BUT, Im just not sure what the problem you were trying to solve by rebasing? are you rebasing because master is ahead of your align-module branch? As far as I can see, you started the branch with dcb7047.. or is that because you rebased?

that is true but see below...

First you dont have to rebase just because master is ahead.

agreed (but still see below).

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.

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.

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.

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.

@alyakin314
Copy link
Contributor Author

moving onto #419 since that rebase worked out correctly. will leave this open 4 temporarily.

@alyakin314 alyakin314 closed this Sep 4, 2020
@alyakin314 alyakin314 deleted the align-module branch September 11, 2020 11:25
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.

create an align module
3 participants