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

Refactor test directory structure to align Chainer's test dir #169

Merged
merged 2 commits into from
Dec 20, 2017

Conversation

kuenishi
Copy link
Member

Addresses #155 .

@kuenishi kuenishi added the test label Dec 19, 2017
@kuenishi kuenishi added this to the v1.1.0 milestone Dec 19, 2017
@iwiwi
Copy link
Contributor

iwiwi commented Dec 19, 2017

Thanks for the PR!

I propose a rule the following rule, which is the same as Chainer: tests for chainermn/AAA/BBB is in tests/chainermn_tests/AA_tests/BBB_tests (What do you think about this rule?). If we adopt the above rule, I would like you to fix the following problems:

  • Please move training_tests/extensions_tests to extensions_tests
  • Please move test_link.py to links_tests (and rename if appropriate)

@kuenishi
Copy link
Member Author

I think we have to follow the Chainer's test directory tree naming rule, that follows namespace, which is fine to me. Also we have to align ChianerMN's namespace to that of Chainer at some timing after 1.1 release. Especially multi node evaluator and checkpointer must be in chainermn.training.extensions , while it's now in chainermn.extensions as well as it's directory position.

@kuenishi
Copy link
Member Author

So far I just pushed required change above.

@iwiwi
Copy link
Contributor

iwiwi commented Dec 20, 2017

Thanks. Previously, we decided not to follow the exact structure of namespaces (e.g., we don't think we need chainermn.training module) to keep our module/directory structures clean and concise. If you have opinions, let us discuss separately.

@iwiwi iwiwi merged commit 3d204d5 into master Dec 20, 2017
@iwiwi iwiwi deleted the refactor-test-dir branch December 20, 2017 01:11
@kuenishi
Copy link
Member Author

ah ok, I understand that decision. I won't do further work on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants