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

Parametric input in single CNN model #163

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

Conversation

sahilyadav27
Copy link

The previous pull request was about testing a parametric model in itself and working on the suggestions received there. This pull request combines the parametric model into the single CNN model and also adds a config file showing the structure to define the parametric model's architecture (fully connected layers)

@@ -0,0 +1,493 @@
# Example config file containing all possible options for CTLearn, whether
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to update the existing example config. You may want to add a TRN with parameter model to the default CTLearn models

Copy link
Author

Choose a reason for hiding this comment

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

okay makes sense

@@ -8,7 +8,10 @@ def single_cnn_model(data, model_params):

# Load neural network model
network_input_img = tf.keras.Input(shape=data.img_shape, name=f"images")
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we want to support MLP only without the CNNs here or in a separate file. There is no need for an additional file I guess. @nietootein What are your thoughts about this?
I ran a quick check. It is possible to request only the parameter list without the images from dl1dh. @sahilyadav27 So I guess you can just check here if images are selected or not with data.img_shape != None

Copy link
Author

Choose a reason for hiding this comment

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

I thought having a separate MLP model would make sense because we might want to add the MLP model to other models as well like CNN_RNN and ResNet. So it would be better to have a common MLP model independent of the image model rather than having it inside the Single CNN model?

Copy link
Author

Choose a reason for hiding this comment

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

This should work now

Copy link
Member

@TjarkMiener TjarkMiener left a comment

Choose a reason for hiding this comment

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

Looks good! Can you please update the default config files? I think you would need to rename engine to engine_cnn to match your changes. At the moment we can wait with a default model for CNN + mlp till we have some solid/best-performing settings for the mlp. However, you can update the existing example config file with the parameter list and mlp settings you have been using for testing, so that new users can see how to use the mlp properly. Other than that everything is fine. We can update the docs separately.

@TjarkMiener TjarkMiener marked this pull request as ready for review December 10, 2022 14:25
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