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

[Contribution] Script for making the necessary files for a new model #86

Merged
merged 20 commits into from
Dec 18, 2020

Conversation

Sahandfer
Copy link
Contributor

model_maker

I've made a script for making the necessary files for a new model that:

  1. Make a model file in cogdl/models/nn/
  2. Make an example file in examples
  3. Add to tests/tasks/ file
  4. Add to match.yml
  5. Remind the user to add their model to README.md

The script can be tested by

$ python scripts/model_maker.py

@Sahandfer
Copy link
Contributor Author

@cenyk1230 Hello, the issues raised by Travis are not from parts I added. Am I expected to fix them for my pull request to be accepted? Let me know if that's the case. Thank you

@cenyk1230
Copy link
Member

Hi @Sahandfer, I have fixed the issues with the code in the master. Please wait for the re-run.

@cenyk1230
Copy link
Member

@Sahandfer Nice job. But I wonder whether the template model should be also tested.

@Sahandfer
Copy link
Contributor Author

Thank you @cenyk1230! I think the template files I created serve as an interface for new models to be added; since they don't have any logic or calculations within them, I don't see it as necessary to test them. However, one improvement that I can think of is modifying the templates in the future after establishing a global style for cogdl models.

@cenyk1230
Copy link
Member

Hi @Sahandfer, I think maybe the template files can be moved to a new folder such as template/ (maybe template/nn/) in the root directory since each file in the cogdl/ will be used for computing coverage.

@Sahandfer
Copy link
Contributor Author

Hello @cenyk1230, I think that's a great suggestion. I have moved the two templates to a new template folder and they are now called: base_model and base_example. I have also updated the script to use these templates.

@cenyk1230
Copy link
Member

@Sahandfer It seems that you moved the wrong model file. I think you should git mv the cogdl/models/nn/template.py rather than cogdl/models/base_model.py.

@Sahandfer
Copy link
Contributor Author

@cenyk1230 Sorry for that. I fixed my mistake; however, the coverage has decreased but I'm not sure what the reason for that is. Thank you and sorry for the trouble.

@cenyk1230
Copy link
Member

Well done!

@cenyk1230 cenyk1230 merged commit e875f12 into THUDM:master Dec 18, 2020
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