Skip to content

Restructuring 2 - Add Utility functions and Run all Tests - #12

Merged
KimSangYeon-DGU merged 6 commits into
mlpack:masterfrom
kartikdutt18:Restructuring-2-a-UtilityFunctions
Jun 2, 2020
Merged

Restructuring 2 - Add Utility functions and Run all Tests#12
KimSangYeon-DGU merged 6 commits into
mlpack:masterfrom
kartikdutt18:Restructuring-2-a-UtilityFunctions

Conversation

@kartikdutt18

@kartikdutt18kartikdutt18 commented May 26, 2020

Copy link
Copy Markdown
Member

This PR is transferred from #8, Since FFN weights some time cause invalid memory access which seems related to mlpack/mlpack#2314. So I am adding utility function to unzip files here and I'll continue working on image dataloader and then we can get back to the LeNet model.
I hope this makes sense.
This PR provides the following :

  1. Unzip Utility Function.
  2. Some bug fixes.
  3. Runs all tests.
    (Probably we should get mnist_test/train in mnist.tar.gz since mnist test and train are fixed).
  4. ListDir Utility function.

Looking forward to your suggestions.
Regards.

CC: @KimSangYeon-DGU

@kartikdutt18
kartikdutt18force-pushed the Restructuring-2-a-UtilityFunctions branch 4 times, most recently from 7a3666b to 07d732aCompareMay 27, 2020 14:15
@kartikdutt18

kartikdutt18 commented May 27, 2020

Copy link
Copy Markdown
MemberAuthor

This works on windows too now, Cleaning commits and outputs.

@zoq

zoq commented May 27, 2020

Copy link
Copy Markdown
Member

(Probably we should get mnist_test/train in mnist.tar.gz since mnist test and train are fixed).

Sure, we can do that, is there anything else you like to add here, this looks pretty good to me.

@kartikdutt18

Copy link
Copy Markdown
MemberAuthor

Sure, we can do that, is there anything else you like to add here, this looks pretty good to me.

I am done with this PR from my side aside from placing mnist_train/test in mnist.tar.gz. If this makes sense I'll ask on the IRC once I incorporate all suggestions.

@zoq

zoq commented May 28, 2020

Copy link
Copy Markdown
Member

Makes sense to me.

@zoqzoq left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From my side this looks ready to go.

@KimSangYeon-DGU

Copy link
Copy Markdown
Member

Looks good to me as well, please let us know if this is ready to go :)

Comment threadtests/dataloader_tests.cpp Outdated
Comment threadutils/utils.hpp Outdated
Completed everything
Fix build, Style fixes next
Use force local for windows while unzipping tar files
Use force local for windows while unzipping tar files
@kartikdutt18
kartikdutt18force-pushed the Restructuring-2-a-UtilityFunctions branch from 3d74ee1 to 9d90a21CompareMay 29, 2020 16:35
Added Utility Function
Style Fix
Fix Typo causing build error in windows
Fix Typo causing build error in windows
Fix const issue for windows
Extract in data folder
Reposition force local
This should work
Print Path in windows for debugging
Print Path in windows for debugging
Print Path in windows for debugging
Stip components of tar
Stip components of tar
strip componenets
Relative Path
Relative Path
Relative Path
Add List Dir utility function
Print
Print again
Print again
Squash this
Update utils.hpp
Squash this
Clean Up
Added more tests
Style Fix
Style Fix
@kartikdutt18
kartikdutt18force-pushed the Restructuring-2-a-UtilityFunctions branch from 585905f to 6ae432eCompareMay 29, 2020 16:56
@kartikdutt18

Copy link
Copy Markdown
MemberAuthor

This is ready from my side. Just tested it everything works fine. I think the builds will result in the same output.
Regards.

@kartikdutt18

kartikdutt18 commented May 30, 2020

Copy link
Copy Markdown
MemberAuthor

Sorry pushed in the wrong branch, reverting now. Fixed now.

@kartikdutt18
kartikdutt18force-pushed the Restructuring-2-a-UtilityFunctions branch from e51aaa9 to 6ae432eCompareMay 30, 2020 10:23
Comment threadtests/dataloader_tests.cpp Outdated
Comment threadtests/dataloader_tests.cpp Outdated
Comment on lines +68 to +69


@KimSangYeon-DGUKimSangYeon-DGUMay 31, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you use 1 newline and add a comment about what we're checking in this block?

And, perhaps, I missed something... can you let me know why the data were distributed like below?
Train : 8,400
Valid: 33,600
Test: 28,000

@kartikdutt18kartikdutt18May 31, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is the same split as mlpack::data::split. Here 0.8 means 80% testing and 20 % training data. mlpack takes in test ratio. I though it would be best not to invert. Let me know if I should do that or keep it same as mlpack. Here is the link for data file.

@KimSangYeon-DGUKimSangYeon-DGUMay 31, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, then, it would be better to change the parameter name ratio to testRatio for clarification in DataLoader class :)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sure, I'll make the change.

@KimSangYeon-DGUKimSangYeon-DGUMay 31, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, kindly update the parameter description as well in here

@KimSangYeon-DGUKimSangYeon-DGUMay 31, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, ok. I thought it's from the same file :)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Should I use validRatio instead I think that makes even more sense.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice. that makes sense to me as well.

@kartikdutt18kartikdutt18May 31, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Right, making the changes.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done. :)

Comment threadutils/utils.hpp
Comment threadtests/dataloader_tests.cpp Outdated
@kartikdutt18
kartikdutt18force-pushed the Restructuring-2-a-UtilityFunctions branch from 66cd0d7 to ed7a643CompareMay 31, 2020 09:14

@KimSangYeon-DGUKimSangYeon-DGU left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, ok, I totally understand 👍 :)

Looks good to me.

@kartikdutt18

Copy link
Copy Markdown
MemberAuthor

Great. Thanks a lot for the reviews.

@mlpack-botmlpack-botBot left a comment

Copy link
Copy Markdown

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. 👍

@KimSangYeon-DGU
KimSangYeon-DGU merged commit f4d4a17 into mlpack:masterJun 2, 2020
@KimSangYeon-DGU

Copy link
Copy Markdown
Member

Thanks! @kartikdutt18 👍

@kartikdutt18

Copy link
Copy Markdown
MemberAuthor

Thanks @KimSangYeon-DGU, @zoq for the reviews and all the help.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@kartikdutt18@zoq@KimSangYeon-DGU