search

MLblog / jads_kaggle

Contains our group's work in various kaggle competitions
MIT License
10 stars 23 forks source link

Functions created or modified to support sampling from hard memory #132

Closed san89 closed 5 years ago

san89 commented 5 years ago

The main functionalities included are:

  1. Utils: functions to save and load pickle objects
  2. helpers: class and functions to create and read chunks of data from memory
  3. source memory: notebook to show how to use the functionalities of the caller class
joepvdbogaert commented 5 years ago

Hi ! Nice job! I only want to make one note. When I approve this PR, it will break the functions in modeling.py and Examples.ipynb. On the long term I do not think this is a problem, because the functions in modeling.py are based on the old way of cv. So I think approving this is fine, but a new task should be to adapt the modeling.py code to the new way of cv (as you already did in the experiments in your notebook).

I'm personally not comfortable with breaking the only working pipeline we have right now. Also, I think it shouldn't be necessary. Functions can either be adapted to work in both cases or a completely separate function can be written to work with the new approach.

san89 commented 5 years ago

Hi ! Nice job! I only want to make one note. When I approve this PR, it will break the functions in modeling.py and Examples.ipynb. On the long term I do not think this is a problem, because the functions in modeling.py are based on the old way of cv. So I think approving this is fine, but a new task should be to adapt the modeling.py code to the new way of cv (as you already did in the experiments in your notebook).

I'm personally not comfortable with breaking the only working pipeline we have right now. Also, I think it shouldn't be necessary. Functions can either be adapted to work in both cases or a completely separate function can be written to work with the new approach.

I agree with Joep, for this reason, I am not touching modeling.py or Examples.ipynb or any other function that has been already developed. Instead, I am calling them on my own code. Therefore the current pipeline will not break.

Christiannewisse commented 5 years ago

Hi ! Nice job! I only want to make one note. When I approve this PR, it will break the functions in modeling.py and Examples.ipynb. On the long term I do not think this is a problem, because the functions in modeling.py are based on the old way of cv. So I think approving this is fine, but a new task should be to adapt the modeling.py code to the new way of cv (as you already did in the experiments in your notebook).

I'm personally not comfortable with breaking the only working pipeline we have right now. Also, I think it shouldn't be necessary. Functions can either be adapted to work in both cases or a completely separate function can be written to work with the new approach.

I agree with Joep, for this reason, I am not touching modeling.py or Examples.ipynb or any other function that has been already developed. Instead, I am calling them on my own code. Therefore the current pipeline will not break.

Sorry for the confusion. Santiago is totally right, this is not breaking the code that is already there.

san89 commented 5 years ago

Hi ! Nice job! I only want to make one note. When I approve this PR, it will break the functions in modeling.py and Examples.ipynb. On the long term I do not think this is a problem, because the functions in modeling.py are based on the old way of cv. So I think approving this is fine, but a new task should be to adapt the modeling.py code to the new way of cv (as you already did in the experiments in your notebook).

I'm personally not comfortable with breaking the only working pipeline we have right now. Also, I think it shouldn't be necessary. Functions can either be adapted to work in both cases or a completely separate function can be written to work with the new approach.

I agree with Joep, for this reason, I am not touching modeling.py or Examples.ipynb or any other function that has been already developed. Instead, I am calling them on my own code. Therefore the current pipeline will not break.

Sorry for the confusion. Santiago's is totally right, this is not breaking the code that is already there.

It is ok Chris, thank you very much for the review!