Skip to content

California Housing Price Predictions Notebook#167

Merged
kartikdutt18 merged 17 commits intomlpack:masterfrom
swaingotnochill:california
Jul 11, 2021
Merged

California Housing Price Predictions Notebook#167
kartikdutt18 merged 17 commits intomlpack:masterfrom
swaingotnochill:california

Conversation

@swaingotnochill
Copy link
Copy Markdown
Contributor

@swaingotnochill swaingotnochill commented Jul 6, 2021

Sorry Marcus :(

@review-notebook-app
Copy link
Copy Markdown

review-notebook-app Bot commented Jul 6, 2021

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

zoq
zoq reviewed Jul 6, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

Some really minor style issues.

Comment thread utils/pandasscatter.hpp Outdated
Comment thread utils/pandasscatter.hpp Outdated
Comment thread utils/pandasscatter.hpp Outdated
Comment thread utils/pandasscatter.hpp Outdated
Comment thread utils/pandasscatter.hpp Outdated
Comment thread utils/impute.hpp Outdated
Comment thread utils/impute.hpp Outdated
Comment thread utils/impute.hpp Outdated
Comment thread utils/impute.hpp Outdated
return 1;
}
}
else{
Copy link
Copy Markdown
Member

@zoq zoq Jul 6, 2021

Choose a reason for hiding this comment

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

Suggested change
else{
else
{

Comment thread utils/impute.hpp Outdated
Py_XDECREF(pFunc);
Py_DECREF(pModule);
}
else{
Copy link
Copy Markdown
Member

@zoq zoq Jul 6, 2021

Choose a reason for hiding this comment

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

Suggested change
else{
else
{

@swaingotnochill
Copy link
Copy Markdown
Contributor Author

swaingotnochill commented Jul 7, 2021

@zoq @kartikdutt18 I also checked other util files and fixed them.Besides, any tools or tips to fix or avoid such style issues?

kartikdutt18
kartikdutt18 reviewed Jul 7, 2021
View reviewed changes
Comment thread ...prediction_with_linear_regression/California_housing_prices_predictions_with_lr_python.ipynb
Comment thread ...prediction_with_linear_regression/California_housing_prices_predictions_with_lr_python.ipynb
Comment thread ...prediction_with_linear_regression/California_housing_prices_predictions_with_lr_python.ipynb
kartikdutt18
kartikdutt18 reviewed Jul 7, 2021
View reviewed changes
Comment thread ...rice_prediction_with_linear_regression/california_housing_price_prediction_with_lr_cpp.ipynb
Comment thread ...rice_prediction_with_linear_regression/california_housing_price_prediction_with_lr_cpp.ipynb
Comment thread ...rice_prediction_with_linear_regression/california_housing_price_prediction_with_lr_cpp.ipynb
@zoq
Copy link
Copy Markdown
Member

zoq commented Jul 8, 2021

@zoq @kartikdutt18 I also checked other util files and fixed them.Besides, any tools or tips to fix or avoid such style issues?

Mainly study https://github.com/mlpack/mlpack/wiki/DesignGuidelines, there might also be an option for you editor to configure the rules we use here - https://github.com/mlpack/jenkins-conf/blob/c859b704d3da9e5f3cd0fc15a6fb408ec3e8e39e/linter/lint.sh#L57-L72.

@swaingotnochill
Copy link
Copy Markdown
Contributor Author

swaingotnochill commented Jul 9, 2021

@zoq @kartikdutt18 I think the commits follows all the style checks now. Maybe we can merge it. Your thoughts?

zoq
zoq reviewed Jul 9, 2021
View reviewed changes
Comment thread mnist_cnn/mnist_cnn.cpp Outdated
Comment thread ...rice_prediction_with_linear_regression/california_housing_price_prediction_with_lr_cpp.ipynb Outdated
Comment thread ...dmission_regression_with_logistic_regression/student-admission-logistic-regression-cpp.ipynb
"display_name":"C++14",
"language":"C++14"
}
"cells": [
Copy link
Copy Markdown
Member

@zoq zoq Jul 9, 2021

Choose a reason for hiding this comment

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

I'm not sure what happened to the file here, but I think it was correct before, so let us remove the changes from the PR.

Copy link
Copy Markdown
Contributor Author

@swaingotnochill swaingotnochill Jul 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ohh..it's not an issue.So, as this file was messed up, I just copied it from different branch. What happened in the process is, all the outputs from jupyter notebook is removed(and nothing else). So, I just ran the notebook once, there is no change in any code.

@swaingotnochill
Copy link
Copy Markdown
Contributor Author

swaingotnochill commented Jul 10, 2021

@zoq @kartikdutt18 See if we can wrap up with this PR.

zoq
zoq approved these changes Jul 10, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, no further comments from my side.

mlpack-bot[bot]
mlpack-bot Bot approved these changes Jul 11, 2021
View reviewed changes
Copy link
Copy Markdown

@mlpack-bot mlpack-bot Bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

kartikdutt18
kartikdutt18 approved these changes Jul 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@kartikdutt18 kartikdutt18 left a comment

Choose a reason for hiding this comment

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

LGTM!

@kartikdutt18 kartikdutt18 merged commit 776e116 into mlpack:master Jul 11, 2021
@swaingotnochill swaingotnochill deleted the california branch July 13, 2021 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zoq zoq zoq approved these changes

@kartikdutt18 kartikdutt18 kartikdutt18 approved these changes

@mlpack-bot mlpack-bot[bot] mlpack-bot[bot] approved these changes

Assignees

No one assigned

Labels

notebook t: added feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@swaingotnochill @zoq @kartikdutt18