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

[MRG] Work on standalone model decoding function #197

Merged
merged 54 commits into from
Sep 24, 2018

Conversation

shuttle1987
Copy link
Member

@shuttle1987 shuttle1987 commented Sep 9, 2018

This is for use with the web API to decode audio files using existing trained models.

@shuttle1987
Copy link
Member Author

I'm thinking it might be good to provide a parameter to the persephone.model.decode function to specify the names of the inputs.

Right now we have the following:

        # TODO These placeholder names should be a backup if names from a newer
        # naming scheme aren't present. Otherwise this won't generalize to
        # different architectures.
        feed_dict = {"Placeholder:0": batch_x,
                     "Placeholder_1:0": batch_x_lens}

        dense_decoded = sess.run("SparseToDense:0", feed_dict=feed_dict)

I propose that we create a parameters for:

  • batch_x name for the location for where the batch data is being fed in
  • batch_x_lens name for the location for where the lengths of the batch data is being fed in

We can default both these to their current placeholder values if they are not specified.

@shuttle1987
Copy link
Member Author

I successfully did a run through with this and it works. I would very much like to create a test case for this.

@shuttle1987
Copy link
Member Author

Just putting in the work to get a test case that can run this through in it's entirety. There's some bugs that appear to be getting uncovered from doing this. At the point there are a couple of failing test cases but this might be just due to the tests not being right.

@oadams
Copy link
Collaborator

oadams commented Sep 14, 2018

The test case written in ff6d958 actually exposes a bit of a nasty edge case, if the model never satisfies this conditional in the training:

                    if valid_ler < best_valid_ler:

Then it never saves a checkpoint file and the evaluation will fail at the end of the Model.train method.

Just initialize best_valid_ler to a large number so that a model always gets saved. It's currently 2.0.

In 5b08f7e I added the following exception in the edge case where evaluation cannot occur:

                # Check we actually saved a checkpoint
                if not self.saved_model_path:
                    raise PersephoneException(
                        "No checkpoint was saved so model evaluation cannot be performed. "
                        "This can happen if the validaion LER never converges.")
                # Finally, run evaluation on the test set.
                self.eval(restore_model_path=self.saved_model_path)

This will give more feedback if there was a problem with the model. Unfortunately the test isn't passing just yet because of the same issue, will likely need a better test corpus.

What corpus are you currently using? If the sine wave test is correct I imagine it should pretty immediately get 100% accuracy.

@shuttle1987
Copy link
Member Author

shuttle1987 commented Sep 14, 2018

What corpus are you currently using? If the sine wave test is correct I imagine it should pretty immediately get 100% accuracy.

A bad one, where the following letters represent the frequencies of piano notes:

  • test: A
  • validation: B
  • training: C

With no overlap from any of these the results ended up being a degenerate case I suspect.

@shuttle1987
Copy link
Member Author

Just initialize best_valid_ler to a large number so that a model always gets saved. It's currently 2.0.

This is something I didn't consider when writing the test case, it might be a good thing to use to reduce the unit test time in this case.

@shuttle1987 shuttle1987 requested a review from oadams September 14, 2018 17:38
@shuttle1987 shuttle1987 changed the title [WIP] Work on standalone model decoding function [MRG] Work on standalone model decoding function Sep 14, 2018
@shuttle1987
Copy link
Member Author

There's quite a few significant changes in this PR and it will unblock work on the web API. As such it would be good to release a new version after this is merged.

shuttle1987 and others added 2 commits September 15, 2018 04:02
- Added TODO
- Minor syntax changes
Copy link
Collaborator

@oadams oadams left a comment

Choose a reason for hiding this comment

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

Nice. Before merging, we need to verify that decoding is working. I've tried doing this: ac03ded
but it's not working so I either need to change the training params, make the training set bigger still, or just test on a wav seen in training.

The last option, just testing on a WAV seen in training, may be best for now.

Also still running the tutorial test.

@shuttle1987
Copy link
Member Author

I think I'd just test on a WAV seen in training, the purpose of these tests was just to have coverage over the non-Tensorflow model parts of the code without having to run the much slower experiment test suite containing real data. Now that said if there's a way of making this sample data test work better that has value in testing the model as well that could be good for a separate test case. The value of a good test of the model is somewhat orthogonal to tests of the code around the model.

@shuttle1987
Copy link
Member Author

I had considered fixing #153 in this PR but I hesitate to make more change here as this PR is at a somewhat stable state and is already over 50 commits. Seeing as this PR will unblock work on the Web API I would like to focus on finalizing anything that's required to merge this one in.

As for integration testing times is this something a GPU based install could help with or are we needing to specifically test on CPU as well?

@oadams
Copy link
Collaborator

oadams commented Sep 17, 2018

I appreciate that this isn't about model testing, but there was already a test_na.py::test_fast() test that used a couple utterances of Na and tested basically the same surrounding architecture, except for decoding from a saved model. The tests for decode here should not just assert that the decoding is None.

In any case, I've put test wavs in training and decoding still outputs some empty lists.

There's two integration tests. One tests the tutorial: you can run this on a CPU pretty quickly: within an hour on the server. The other tests a full model and basically replicates results from the paper and takes about a day. I always use a GPU for the latter but have only run it once or twice: if the first one works then there's an extremely unlikely the second one would break.

@shuttle1987
Copy link
Member Author

shuttle1987 commented Sep 17, 2018

In any case, I've put test wavs in training and decoding still outputs some empty lists.

That's unfortunate if that isn't working, I'd hoped to get some sensible output here.

@oadams
Copy link
Collaborator

oadams commented Sep 17, 2018

Note, it doesn't always output empty lists, but it's. It's just clear a better corpus and training procedure is needed. I can fix this sometime. The best thing to do would just be to add a test for the decode() function into the integration test that tests the tutorial, that way we can confirm its working. We can go ahead and merge this in and release to forge ahead with the web API.

@shuttle1987
Copy link
Member Author

The best thing to do would just be to add a test for the decode() function into the integration test that tests the tutorial, that way we can confirm its working

I agree that this is the best way forward, if this is something you could do that would be great.

Would a JSON formatted data output from the model description help this? If so I can put those changes into this PR as well.

@oadams
Copy link
Collaborator

oadams commented Sep 17, 2018

Yeah, I'll do it.

Yes to JSON if it's a super quick job. Something formatted better than the current text files would be good, but that said this is for human readability and those text files do already do the job.

@oadams oadams merged commit 1999ca5 into persephone-tools:master Sep 24, 2018
This was referenced Oct 23, 2018
Sign up for free to 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.

Create minimal test corpus Filesystem artifacts need tests Add type signatures to just about everything
2 participants