Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt MNIST example to use the original lecun data files #97

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

jrp2014
Copy link

@jrp2014 jrp2014 commented Jan 26, 2020

This version downloads and uses the MNIST data files, converting the data from Word8 to Double. The runMNIST.sh script does the work.

Unfortunately, it still runs out of memory on my Ubuntu VM...

Anyway, I hope that it's of some use.

@jrp2014
Copy link
Author

jrp2014 commented Jan 26, 2020

The CI seems to confirm that it's not just an issue with my setup.

@erikd
Copy link
Collaborator

erikd commented Jan 26, 2020

The Data.Semigroup import needs to be wrapped in some CPP goop.

Let me do that in a separate PR.

Copy link
Collaborator

@erikd erikd left a comment

Choose a reason for hiding this comment

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

In general PRs should change only what absolutely must be changed.

examples/main/mnist.hs Outdated Show resolved Hide resolved
'D3 12 12 18, 'D3 4 4 18, 'D3 4 4 18,
'D1 288, 'D1 80, 'D1 10 ]
type MNIST
= Network
Copy link
Collaborator

Choose a reason for hiding this comment

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

More semantically NULL changes.

@erikd
Copy link
Collaborator

erikd commented Jan 26, 2020

I am actually wondering why we don't just update the documentation.

@erikd
Copy link
Collaborator

erikd commented Jan 27, 2020

For PR branches like this one, you really should rebase rather than merge,

examples/main/gan-mnist.hs Outdated Show resolved Hide resolved
examples/main/gan-mnist.hs Outdated Show resolved Hide resolved
examples/main/gan-mnist.hs Outdated Show resolved Hide resolved
examples/main/gan-mnist.hs Outdated Show resolved Hide resolved
runGAN-MNIST,sh Outdated
@@ -0,0 +1,26 @@
stack=cabal
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that a comma in that file name or a period?

Should probably also use a #!/bin/bash -eu at the top as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, can stack and cabal be use interchangablly there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this shell script can be made quite a bit nicer. I can do that but may not get a chance before the weekend.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good. The script is an extract of a retread of a retread :-)

examples/main/gan-mnist.hs Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@jrp2014
Copy link
Author

jrp2014 commented Jan 29, 2020

The CI fails because gan-mnist takes too long to generate 15 examples. mnist itself still uses too much memory to run.

@erikd
Copy link
Collaborator

erikd commented Feb 1, 2020

Where is this this PR going? If this is ever to be merged it should be rebased against master and commits like "ci typo" need to be squashed down.

@jrp2014
Copy link
Author

jrp2014 commented Feb 1, 2020

Thanks @erikd. I want to find time over the w/e to kick the tyres some more:

  • better run script (so that args can be passed)
  • use that to limit travis.yaml test runs so that they pass within time and space constraints
  • improve formatting / intelligibility of output
  • try some different example models in mnist
  • a couple of lines of documentation on how to run, easily
  • optimization (eg, by profiling) for time / space improvements
  • git hygiene

Are you going to be able to refactor the script?

@erikd
Copy link
Collaborator

erikd commented Feb 1, 2020

Probably best if I wait until you finish what you are working on and then assuming everything is ok, I will do done final clean up run

.hlint.yaml Show resolved Hide resolved
Copy link
Collaborator

@erikd erikd left a comment

Choose a reason for hiding this comment

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

Big PRs with mainy unrelate changes are far more difficult to review than a series of small PRs that only change one thing.

There is already one large, long running PR against Grenade that is likely to never be merged because it contains a huge number of unrelated changes and that PR branch has diverged from master. Changes on that branch will be need to be cherry picked, tested, evaluated indvcidiually which is significantly more work that if they had been submitted as s stream of small simple PRs.

, split
, zlib

executable iris
Copy link
Collaborator

Choose a reason for hiding this comment

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

A new example is a great idea, but should probably be a separate PR from the other changes you are making.

Having them a separate make reviewing both PRs easier.

Copy link
Author

Choose a reason for hiding this comment

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

Understood. In this case the changes are focused on making the examples run (which they now do, as you can see from the travis output). I think that I've got to a stage where the pieces are in place. If you want to improve the run scripts, I think that that would be good. I'd be happy to flesh out some of the documentation, if that would help.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not too sure it the examples should run in CI. Will have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants