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

CASC #599

Merged
merged 119 commits into from May 20, 2021
Merged

CASC #599

merged 119 commits into from May 20, 2021

Conversation

@netlify
Copy link

netlify bot commented Dec 7, 2020

Deploy Preview for graspologic failed.

Built with commit ab13228

https://app.netlify.com/sites/graspologic/deploys/60a6ae9221c0bc0007ac05c2

@j1c j1c marked this pull request as draft December 7, 2020 16:59
@loftusa loftusa changed the title [WIP] CASC CASC Feb 27, 2021
@loftusa
Copy link
Collaborator Author

loftusa commented Feb 27, 2021

@bdpedigo Ok, I think code is working well enough to look at it? still a bit early and there's a lot of debugging code floating around, but feel free to take a first pass if you'd like.

a few things:

  • I implemented the tuning algorithm over the course of the last few days
  • Code for grabbing the tuning parameter is still pretty messy, I'm planning to clean it up
  • I currently save the laplacian, the covariates, and a few functions of them into the class instantiation object in fit. That's somewhat temporary, planning on changing that as well
  • the tuning parameter algorithm needs to fit within a range a bunch of times and then run k-means repeatedly (then return whichever one has the smallest k-means inertia). To do that, I need to re-embed repeatedly, but I don't want to call self.reduce_dim_ a bunch. Just wrote my own code for it, but feels like it's repeating code from the superclass a bit
  • haven't made the updated figure yet (it's gonna take a long time to run)
  • on that note, current iteration of the tuning algorithm takes about 10 seconds to fit on a 300-node graph, since it has to run k-means in a loop over a tuning space. Can't really think of a way to speed it up.
  • tests broke since I changed some stuff after implementing the tuning algorithm, shouldn't take too long to get them up and running again though

@bdpedigo
Copy link
Collaborator

bdpedigo commented May 2, 2021

@loftusa mind resolving any comments you've addressed and need no further discussion?

@loftusa
Copy link
Collaborator Author

loftusa commented May 2, 2021

@loftusa mind resolving any comments you've addressed and need no further discussion?

done

@bdpedigo
Copy link
Collaborator

bdpedigo commented May 5, 2021

just a heads up that tests aren't passing anymore

@loftusa
Copy link
Collaborator Author

loftusa commented May 6, 2021

just a heads up that tests aren't passing anymore

fixd

@bdpedigo
Copy link
Collaborator

bdpedigo commented May 6, 2021

what did you end up doing regarding our conversation about checking for directed graphs?

@loftusa
Copy link
Collaborator Author

loftusa commented May 7, 2021

what did you end up doing regarding our conversation about checking for directed graphs?

#599 (comment)

@bdpedigo
Copy link
Collaborator

@loftusa no rush on my end but can you just re-request review when tests are passing and everything looks good to go?

@loftusa loftusa requested a review from bdpedigo May 20, 2021 19:21
@loftusa
Copy link
Collaborator Author

loftusa commented May 20, 2021

@loftusa no rush on my end but can you just re-request review when tests are passing and everything looks good to go?

ok, tests are passing, except for the deploy preview, but it looks like that's some issue with the new graspologic-native version that got released two days ago rather than anything in this PR

(maybe I'm wrong?)

@bdpedigo bdpedigo merged commit 2125f27 into graspologic-org:dev May 20, 2021
@bdpedigo
Copy link
Collaborator

thanks @loftusa 🎉

@loftusa loftusa deleted the casc branch May 20, 2021 22:42
@loftusa
Copy link
Collaborator Author

loftusa commented May 20, 2021

thanks @loftusa 🎉

Woo!

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.

implement CASC
6 participants