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

Add directory support for generator to_configure parameter #119

Merged
merged 13 commits into from
Jan 7, 2022

Conversation

MattToast
Copy link
Member

Changes in files.py

  • Create aTaggedFilesHierarchy class to build a tree of directories of files that need to be configured
  • Create a EntityFiles.tagged_hierarchy property, to assign a TaggedFilesHierarchy object to after checking the EntityFiles.tagged files
  • Build EntityFiles.tagged_hierarchy in EntityFiles._check_files method

Changes in generator.py

  • In the Generator._write_tagged_entity_files Replace loop over list of file paths with a DFS over TaggedFilesHierarchy
  • Any files found in DFS are added to a list to be files to be configured

@MattToast MattToast linked an issue Dec 16, 2021 that may be closed by this pull request
@MattToast MattToast changed the title Gen dir support Add directory support for generator to_configure parameter Dec 16, 2021
@MattToast MattToast requested review from al-rigazzi and removed request for mellis13 December 17, 2021 21:18
Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

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

Very good work. Mostly style- and docstring-related comments. I like the TaggedFilesHierarchy class and its mechanisms. Good work!

smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/generation/generator.py Outdated Show resolved Hide resolved
smartsim/generation/generator.py Outdated Show resolved Hide resolved
@MattToast
Copy link
Member Author

MattToast commented Dec 21, 2021

Changes:

  • Addressed use of single character non-counting variables
  • More descriptive docstrings (with fewer typos!)

Copy link
Contributor

@Spartee Spartee left a comment

Choose a reason for hiding this comment

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

Couple changes requested, but overall looks great. Using Pathlib could make your life a little easier here, but I'll leave that up to you.

Some things I would like to see are more tests that cause the errors to be raised. (Some adversarial examples)

Let me know if you have any questions.

smartsim/entity/files.py Show resolved Hide resolved
smartsim/entity/files.py Show resolved Hide resolved
smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/entity/files.py Outdated Show resolved Hide resolved
smartsim/entity/files.py Show resolved Hide resolved
@Spartee Spartee added the area: generation Issues related to Experiment file generation label Jan 7, 2022
@Spartee Spartee self-requested a review January 7, 2022 18:33
Copy link
Contributor

@Spartee Spartee left a comment

Choose a reason for hiding this comment

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

Approved!! LGTM. Great work. thanks for responding to the feedback!

@Spartee Spartee dismissed al-rigazzi’s stale review January 7, 2022 18:35

Overriding review as Al is on vacation

@MattToast MattToast merged commit 1f2c3c4 into CrayLabs:develop Jan 7, 2022
@MattToast MattToast deleted the gen-dir-support branch January 7, 2022 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: generation Issues related to Experiment file generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support directories in generator to_configure parameter
3 participants