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

Dead code? #21

Open
martingasser opened this issue Jun 22, 2022 · 5 comments
Open

Dead code? #21

martingasser opened this issue Jun 22, 2022 · 5 comments

Comments

@martingasser
Copy link

Hi,

first of all, congratulations to this work!

During examining the model, I noticed that x_contours is not used and overwritten after

x_contours = tfkl.Conv2D(
n_filters_contour,
(5, 5),
padding="same",
kernel_initializer=_initializer(),
kernel_constraint=_kernel_constraint(),
)(x)
x_contours = tfkl.BatchNormalization()(x_contours)
x_contours = tfkl.ReLU()(x_contours)

I also wanted to ask if you plan on releasing the script used for training the model as well.

Thanks,
Martin

@drubinstein
Copy link
Contributor

Hi @martingasser !

Good find. Feel free to PR a change or I will when I have time to update the code.

Regarding releasing the code for training, we intend to push it sometime during the summer. Thanks for your patience.

@sonovice sonovice mentioned this issue Jan 13, 2023
@eunkoh
Copy link

eunkoh commented May 25, 2023

Hi @drubinstein
I can see your branch with train.py but it's hard to use this branch without any example or explanation. Could you share any examples for the branch? Basically, I am interested in using my own data for training, but it's hard to reproduce the training with your data as well. Any information would be greatly appreciated. Many thanks.

https://github.com/spotify/basic-pitch/blob/drubinstein/wip-training/basic_pitch/train.py

Hi @martingasser !

Good find. Feel free to PR a change or I will when I have time to update the code.

Regarding releasing the code for training, we intend to push it sometime during the summer. Thanks for your patience.

@eunkoh
Copy link

eunkoh commented May 25, 2023

@drubinstein For example, do I need *tfrecord for train/valid split for training? line 232 in https://github.com/spotify/basic-pitch/blob/drubinstein/wip-training/basic_pitch/dataset/tf_example_deserialization.py

@bgenchel
Copy link
Collaborator

bgenchel commented May 26, 2023

Hey @eunkoh, I'm planning on working on the training code in the near future, so i'll try to answer your questions to the best of my ability. I think it's worth pointing out that the code in that branch is still 'wip' as labeled, so we're aware on our side that it doesn't work in it's current form.

As for your question about *tfrecord, I'm guessing that you're referring to the specific argument in os.join. This just specifies the file extension that should be read in the preceding folder in the path (the split var) . The split var looks to contain a string, either "valid" or "train", which will determine what generator function will be used in the subsequent lines.

Please let me know if i'm misunderstanding your question and clarify further if so!

@TeeJayBaker
Copy link

Hey @drubinstein @rabitt,
Sorry to rehash this after so long, just a question as to the model structure, as I'm trying to transfer the weights dict to port to a PyTorch model.

Was in intended to be the reduced version, with the three skipped layers that are still present in the code? Or was it just a happy accident that it happened to work very well without them? As the model structure diagram in the paper features the original Conv-BatchNorm-ReLU that's been skipped.

If so would you consider retraining with the original intended model structure and see if the model accuracy improves?
Any clarification would be much appreciated!

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

No branches or pull requests

5 participants