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

Docstring changes and some suggestions #191

Merged
merged 11 commits into from
Jan 20, 2022
Merged

Conversation

seankmartin
Copy link
Contributor

Hi,

Amazing repository, delighted to see such an easy-to-use renderer for neuroscience.

I have been using this quite a lot recently, and have a few suggested edits if you are interested in them.
I'm open to any thoughts and further edits on these of course!

Best wishes,
Sean

Allows for usage of
import brainrender
brainrender.Atlas

which I find quite handy and think others may too
fucntionally equivalent to old, but think super() is clearer
+ more future proof
The latest version of bg-atlasapi (from Github) has no
print_authors argument, so the old code crashed on Atlas init
This comes from a personal use that I thought might help others.
I had many GB of data in RAM when a crash happened in brainrender.
Rich tried to display my local variables to terminal, and couldn't.
Disabling rich printing is best here.

There may be a better solution to this problem, but since the
previous code loading the rich style traceback at init of brainrender
this was the best I could think of.
@FedeClaudi
Copy link
Collaborator

Hi,

Thanks!

The comments look really good (so many typos haha), thank you so much for sending this in. The screenshot.py example is also very useful.

The only thing I'm not sure is settings.RICH_TRACEBACK to disable the formatting of error messages. Is there any reason why you'd like to switch those off?

@seankmartin
Copy link
Contributor Author

Great, glad to hear it!

The settings.RICH_TRACKBACK is because I am using brainrender along with another application where I have many GB of local variables.
So if a crash happens during runtime (which it often does because I write dumb code haha) then the rich traceback tries to display all the local variables to terminal, which it can't do because there are too many (well it probably could do it, but it would take a very long time).
This causes the program to hang and needs manual killing.
Plus it is very hard to find the cause of the bug without running again in debug mode.

So I suggested the ability to suppress the high detail traceback in case any others run into a similar issue!
Though perhaps there is another better solution to suppressing the local variables if they are very big?
If it seems too niche that's no worries at all, I'll just keep it on my local branch

@FedeClaudi
Copy link
Collaborator

I see, makes sense. So pyinspect renders errors similar to the way rich does (and it uses rich). It has an option to show/hide locals in the error trace or even to decide how many levels of locals to show in the stack.

I think the best option would be to have show_locals = settings.DEBUG, this way it will generally not show them unless the users starts brainrender in debug mode, in which case it might want to have that extra information.
This way we avoid adding an additional settings that users might be confused about.

Btw, in general I think that if after importing brainrender in your code you use install_traceback with the parameter you need it will override brainrender's.

@seankmartin
Copy link
Contributor Author

seankmartin commented Jan 20, 2022

Yeah, that sounds perfect, I think what you suggest makes much more sense.

Also glad to hear about the ability to override the show_locals parameter if I call install_traceback again, that would be great for me, so I'll definitely try that out too - thanks!

@FedeClaudi
Copy link
Collaborator

Awesome.

If you want to add a commit with the suggested settings.DEBUG change I'd be very happy to merge.

@seankmartin
Copy link
Contributor Author

Ok added that, hopefully what you had in mind, but please let me know if not.
Also realised that I was missing a closing newline in the screenshot example file, so added that as well.

@FedeClaudi
Copy link
Collaborator

Looks good. Thanks for the PR!

@FedeClaudi FedeClaudi merged commit ae92a40 into brainglobe:master Jan 20, 2022
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.

None yet

2 participants