search

UBC-MDS / CrossPy

A package for cross-validation in Python
MIT License
1 stars 4 forks source link

Feedback for Milestone 2 #21

Open arthursunbao opened 6 years ago

arthursunbao commented 6 years ago

Hi All,

Nice work for the milestone 2 and here is my comments for your code:

  1. what is the cache folder used for?

  2. For test files, suggest to have comments for each function as well

  3. Suggest to refer to https://google.github.io/styleguide/pyguide.html for Python coding style improvement with how much space with each line and indentation issues

  4. For file crosspy.py. I see only two contributors. So where is the other contributor’s work?

  5. crosspy.py: line 36 to line 55, that can be combined into one line of code

  6. crosspy.py: line 82, Why X variable is named for capital letter? Suggest to refer to https://google.github.io/styleguide/pyguide.html for Python coding style

  7. crosspy.py: line 100 to line 121, same issue with line 5

  8. crosspy.py: function split_data, where is the return value for your function?

Regards Jason

raffrica commented 6 years ago

Hi Jason @arthursunbao ,

With regards to number 4.. same story as the last time you pointed it out. I'm not a listed contributor because I didn't actually do the merge.. I did write the code for cross_validation which is the majority of that file though.

arthursunbao commented 6 years ago

Got it. Thanks

2018-03-15 12:12 GMT-07:00 Daniel Raff notifications@github.com:

Hi Jason @arthursunbao https://github.com/arthursunbao ,

With regards to number 4.. same story as the last time you pointed it out. I'm not a listed contributor because I didn't actually do the merge.. I did write the code for cross_validation which is the majority of that file though.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/UBC-MDS/CrossPy/issues/21#issuecomment-373491075, or mute the thread https://github.com/notifications/unsubscribe-auth/AHyUTXEMgrqCZsiDtXlqm821FN26SoUNks5ter0ygaJpZM4SreSf .

ShunChi100 commented 6 years ago

@arthursunbao Hi Jason, thank you very much for the feedback. We have addressed the issues below. Please let us know what you think, so we can update accordingly.

  1. what is the cache folder used for? It is a cache folder for pytest.
  2. For test files, suggest to have comments for each function as well We think that these test functions are self-descriptive enough so no comments are required. Let us know if you think differently.
  3. Suggest to refer to https://google.github.io/styleguide/pyguide.html for Python coding style improvement with how much space with each line and indentation issues We have modified styles so they more consistent and clean.
  4. For file crosspy.py. I see only two contributors. So where is the other contributor’s work? As pointed out by Dan. Please see commits.
  5. crosspy.py: line 36 to line 55, that can be combined into one line of code We purposely separate cases for raising errors, so they are clear when returning error messages. Foe example, X, y are separate because y is a one-column dataframe and X typically is a multi-column dataframe. It is easier for people to have y as the wrong input type because people are used to y being a numpy vector which is not allowed here. Whereas
  6. crosspy.py: line 82, Why X variable is named for capital letter? Suggest to refer to https://google.github.io/styleguide/pyguide.html for Python coding style capital X is commonly used in machine learning. So keep X instead of x. We have modified styles so they more consistent and clean.
  7. crosspy.py: line 100 to line 121, same issue with line 5 As discussed in item 5
  8. crosspy.py: function split_data, where is the return value for your function? It is a generator function, so yield is the "return value"
arthursunbao commented 6 years ago

Thank you.

2018-03-18 2:10 GMT-07:00 Shun Chi notifications@github.com:

@arthursunbao https://github.com/arthursunbao Hi Jason, thank you very much for the feedback. We have addressed the issues below. Please let us know what you think, so we can update accordingly.

  1. what is the cache folder used for? It is a cache folder for pytest.
  2. For test files, suggest to have comments for each function as well We think that these test functions are self-descriptive enough so no comments are required. Let us know if you think differently.
  3. Suggest to refer to https://google.github.io/styleguide/pyguide.html for Python coding style improvement with how much space with each line and indentation issues We have modified styles so they more consistent and clean.
  4. For file crosspy.py. I see only two contributors. So where is the other contributor’s work? As pointed out by Dan. Please see commits.
  5. crosspy.py: line 36 to line 55, that can be combined into one line of code We purposely separate cases for raising errors, so they are clear when returning error messages. Foe example, X, y are separate because y is a one-column dataframe and X typically is a multi-column dataframe. It is easier for people to have y as the wrong input type because people are used to y being a numpy vector which is not allowed here. Whereas
  6. crosspy.py: line 82, Why X variable is named for capital letter? Suggest to refer to https://google.github.io/styleguide/pyguide.html for Python coding style We have modified styles so they more consistent and clean.
  7. crosspy.py: line 100 to line 121, same issue with line 5 As discussed in item 5
  8. crosspy.py: function split_data, where is the return value for your function? It is a generator function, so yield is the "return value"

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/UBC-MDS/CrossPy/issues/21#issuecomment-373983129, or mute the thread https://github.com/notifications/unsubscribe-auth/AHyUTTgr-3xMGekGTO6ka1yxOdpFmpPMks5tfiSdgaJpZM4SreSf .