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

Refactoring structures/.. #14

Merged
merged 5 commits into from
May 28, 2020
Merged

Refactoring structures/.. #14

merged 5 commits into from
May 28, 2020

Conversation

FedeClaudi
Copy link
Contributor

Changes to structures/simple_tree.py and structures/structure_tree.py:

  1. removed obsolete methods to remove dependency on allensdk
  2. made most methods that expected and return a list more flexible, now you can pass a single item and they will return a single item
  3. move print_structures and print_structures_tree from core.Atlas to structures_structures_tree.py to keep Atlas cleaner.

@@ -389,3 +396,93 @@ def path_to_list(path):
return list(path)

return [int(stid) for stid in path.split("/") if stid != ""]

def print_structures(self, to_file=False, save_filepath=None):
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a convenient function and I see the point of having it here; I am a splitting maniac and I would keep it as a separate function then used here, put it's a matter of taste - as long as this class does not get too long

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am a splitting maniac

You really are :P

I prefer keeping the number of files / classes low at the cost of slightly longer classes. Especially for stuff like this which is behind the scenes and we won't have to edit it often if at all.

I guess we can find a compromise? :)

Copy link
Member

Choose a reason for hiding this comment

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

I would agree on a maximum number of lines per class 😄 500?

@vigji vigji merged commit e06bf83 into master May 28, 2020
willGraham01 pushed a commit that referenced this pull request Feb 14, 2024
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.

3 participants