leonjia0112 / ec601_passenger_screening

1 stars 1 forks source link

Code Review - Abhay Sarda #6

Closed abhaysarda closed 6 years ago

abhaysarda commented 6 years ago

Hi Guys,

Most of the code was easily readable. Comments and explanations were a little inconsistent, but since everyone has mentioned it in the issues, I think it's fine for now.

In the peijia branch, I especially noticed that there are a lot of numbers you naturally used for filtering the images to usable ones, but most of them are magic numbers, without any explanation for the choice of numbers. In Vikram's branch, which is the source of the image processing files, all of the files are duplicated once. The SCC.log has mainly the missing module information, which I think will be fixed shortly, since it's being used on BU SCC, so the module conflict. Lastly, in the image preprocessing file, if you could comment why you are using a particular tensor flow model, it might be helpful to people who want to further your work in the future.

One thing I noticed was that the master branch didn't have any parts of the code for most of the components of the project. I understand that most of the code is still being written, hence it's in each user's directory. But since master branches are a close approximation of what stage the product dev. is at, it might be better if you could put a really basic version of all components on the master page, and refer the right path in the readme files to the most current version, in each user's branch.

In the existing readme's, it might be good to cite what code is from other users than the ones in the group if any. I assumed all the code has been written for this project alone, but if any of it is from Kaggle projects or any other user's repo, it should be cited as a source. Also, I meant to ask you in the class as well, but what exactly are the imaging techniques that gave the data for each user? Are they millimeter scanning or backscatter machines? Lastly, it would be great if you could have a readme to show what are the current challenges you are facing for easier understanding.

I apologize if I was too harsh, I am sure you guys will do a great job with it! Best of luck! Warm regards, Abhay

abhaysarda commented 6 years ago

Update Issue 4 - Abhay

gvikram31 commented 6 years ago

Hi Abhay,

Thank you so much for the detailed feedback. You did a detailed analysis of our project.

Below are the answers for some of the question:-

Thank you again for the feedback.

--Vikram

abhaysarda commented 6 years ago

Hi Vikram,

Thanks for your response! I understand that all of the issues are common in the development phase. I am sure you guys will do a great job by the end.

Best of luck with the project!

Regards,

Abhay

From: Vikram V notifications@github.com Reply-To: leonjia0112/ec601_passenger_screening reply@reply.github.com Date: Friday, November 17, 2017 at 5:40 PM To: leonjia0112/ec601_passenger_screening ec601_passenger_screening@noreply.github.com Cc: abhaysarda asarda@bu.edu, Author author@noreply.github.com Subject: Re: [leonjia0112/ec601_passenger_screening] Code Review - Abhay Sarda (#6)

Hi Abhay,

Thank you so much for the detailed feedback. You did a detailed analysis of our project.

Below are the answers for some of the question:- Currently, we are using the magic numbers a lot as we have to separate the different parts of an image and all of the images have exact same structure. Also, we were extracting only a few of images from whole data as it is quite big in size (>70GB). I have used some part of Kaggle kernel and cited that in comments. Currently, we are in development phase we are focusing on the proof of concept part so we did write comments about the full algorithm as we aren't sure how good it will perform. But surely we will make the final version much more easy to read and understandable. Readme file is not having proper details about the project and navigation information about branches which will be taken care soon. About imaging technique which provided us the data for each user, we don't know about them as TSA only share images for this competition they didn't tell methodology about how they producing images. We are mainly focused on algorithm part. Thank you again for the feedback.

--Vikram

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.