Skip to content

Conversation

@amitgalor18
Copy link
Collaborator

Adding googlenet model to models, with a transform to the spectrograms to make them 3 channeled (RGB) in order to use a pretrained googlenet, based on the JASA paper. Creating conf files to enable experiments with the googlenet model

amitgalor18 and others added 30 commits January 10, 2023 18:51
… paper and added a method for repeating the spectrogram tensor in 3 channels
# Conflicts:
#	requirements.txt
#	soundbay/conf_dict.py
@CLAassistant
Copy link

CLAassistant commented May 29, 2023

CLA assistant check
All committers have signed the CLA.

@amitgalor18 amitgalor18 added the ci-test label to trigger CI flow label May 29, 2023
@Z30G0D Z30G0D added ci-test label to trigger CI flow and removed ci-test label to trigger CI flow labels Jun 2, 2023
Copy link
Collaborator

@Z30G0D Z30G0D left a comment

Choose a reason for hiding this comment

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

Great PR!
Important, your test seems to be failing on one of your new imports from torchvision.
Try to consult this post:
https://stackoverflow.com/questions/70998767/no-module-named-torchvision-models-utils

Although I am concerned about this message,

Note that this syntax is only for higher versions of PyTorch.

We might need to upgrade torch for this.

@@ -0,0 +1,8 @@
# @package _global_
Copy link
Collaborator

@Z30G0D Z30G0D Jun 2, 2023

Choose a reason for hiding this comment

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

Why does this file exist?
Should there be so many new yaml files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that different types of experiments (e.g. with many different parameters such as model, preprocessing, optimizer, and not just one different parameter) are easer to handle when they have their own main. If it's only one parameter like a dataset path it's simple enough to add an override to the train command line. The new "optim", "preprocessors" and "model" yamls are of course required.

But on a related note, I may have misunderstood the changes you made in the latest PR to the hierarchy, I thought that every "runs" conf file should now be in the parent folder in order to reach all the relevant yamls, so I moved main, main_inference and the parallel files for the googlenet manatees experiment up to the conf folder.

wandb.log(log_specs, commit=False)
#avoid spectrogram upload if using 3 channel spectrograms (in googlenet), upload only wavs
if audio.size()[1] == 3:
log_wavs = {f'First batch {flag} original wavs': list_of_wavs_objects}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to notify user somehow about this, he will be surprise.
Maybe use a warning or info statement to notify about this.
Should appear only once or twice during the run though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea, I'll check how to do that

@Z30G0D Z30G0D requested a review from mosheman5 June 2, 2023 14:09
@amitgalor18 amitgalor18 added ci-test label to trigger CI flow and removed ci-test label to trigger CI flow labels Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-test label to trigger CI flow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants