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

Saving #22

Merged
merged 8 commits into from
Dec 3, 2018
Merged

Conversation

YoungFaithful
Copy link
Collaborator

@YoungFaithful YoungFaithful commented Nov 28, 2018

ClustResultBest implemented, ClustResultAll as option in run_clust, deleted kwargs as deprecated in the other functions

@holgerteichgraeber Do I need to open that after the other one is approved or will it work as soon as you accepted the other one?

…eleted kwargs as deprecated in the other functions
@holgerteichgraeber
Copy link
Owner

I thought it should work like you did, but have a look at the Files Changed tab, it somehow shows all the old changes. Could you close this one and add a new PR? There must be a way to do several PRs parallel, if you come across something, let me know, I'll have a look as well.

@holgerteichgraeber
Copy link
Owner

Or can you try git pull holger master into the Saving branch? That should do a fast forward merge and we could check if it solved the issue.

@YoungFaithful YoungFaithful reopened this Nov 28, 2018
src/utils/load_data.jl Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@YoungFaithful YoungFaithful left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments

src/utils/datastructs.jl Show resolved Hide resolved
src/clustering/run_clust.jl Outdated Show resolved Hide resolved
@holgerteichgraeber
Copy link
Owner

I appreciate the comments on some of the changes, thank you!

Copy link
Owner

@holgerteichgraeber holgerteichgraeber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • What do you think about moving the function attribute_weighting() from run_clust.jl to utils/utils.jl ? I feel that run_clust() should only keep the clustering functions themselves.
  • Could you change Line 147 in run_clust() to results_ar = Array{ClustResult,1}(undef,length(n_clust_ar)), somehow I cannot make that comment in the code review (probably because that part of the file did not change)?

src/utils/load_data.jl Outdated Show resolved Hide resolved
src/utils/datastructs.jl Outdated Show resolved Hide resolved
src/utils/datastructs.jl Show resolved Hide resolved
src/clustering/run_clust.jl Outdated Show resolved Hide resolved
src/clustering/run_clust.jl Outdated Show resolved Hide resolved
@YoungFaithful
Copy link
Collaborator Author

Should be done

Copy link
Collaborator Author

@YoungFaithful YoungFaithful left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done from my perspective

Copy link
Collaborator Author

@YoungFaithful YoungFaithful left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

examples/workflow_example_bat.jl Show resolved Hide resolved
@holgerteichgraeber holgerteichgraeber merged commit f651581 into holgerteichgraeber:master Dec 3, 2018
@holgerteichgraeber
Copy link
Owner

@YoungFaithful : Just pushed a small fix in workflow_example_bat to master, everything should be working now.

@YoungFaithful YoungFaithful deleted the Saving branch December 4, 2018 07:53
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.

None yet

2 participants