Remove auto-print upon BrainGlobeAtlas construction

[nickdelgrosso]

Hi,

This is a very minor change request, so no worries if it's ignored, but I noticed that BrainGlobeAtlas() prints itself when it is initialized. This is different than the convention, and ends up adding unnecessary output to the standard output during normal operations. For my use case (wrapping BrainGlobeAtlas in an application), it would mean suppressing the bg-atlasapi output in order to have control over my application's output. It also leads to a double-printing when someone explicitly requests output in a jupyter notebook cell, as in:

atlas = BrainGlobeAtlas("allen_mouse_25um")
atlas

The pull request just removes the extra print statement in BrainGlobeAtlas.init(), leaving the user to decide if they want to see the output themselves.

Best wishes,

Nick

[adamltyson]

You should be able to use the print_authors argument:

atlas = BrainGlobeAtlas("allen_mouse_25um", print_authors=False)

Although I agree that it is a bit annoying, and maybe print_authors should be False by default.

[vigji]

I had opted for that in a maybe overly scrupulous attempt to make sure brainglobe does not “steal” credits from the atlas authors. I agree it is not a very kosher choice, and I let the argument for that reason; but happy to change if you guys think so, and we can change it back if we have any complaints. Happy to merge if you decide so, just remove the argument as well in that case maybe

[nickdelgrosso]

print_authors argument removed.

[adamltyson]

@FedeClaudi what do you think? I think I'd rather keep this functionality, but just make it obvious to users that they can suppress it if they like.

I agree with @vigji that we should always try to "divert" credit back to the original atlas authors where possible.

[FedeClaudi]

I think as long as we make it abundantly clear that we haven't 'made' the atlases in the docs there's no need to have the authors name printed out everytime a class is used, I'm sure most people will suppress it anyways and it can be annoying for people building software using the API. We say it everywhere, including the paper, that it's an API built on top of other people's data and the atlases come with metadata clearly stating that.

So I'd switch it off by default.
We can have it be printed out when an atlas is downloaded/updated, we can also make a neat print out of the atlas metadata (e.g.: https:/FedeClaudi/pyinspect/blob/master/media/showpi.png) to show upon download or from a .info() kind of method.

[adamltyson]

Cool, so we keep the method, but turn it off by default? Then at some point make it fancy?

[FedeClaudi]

I'd say so... happy to fancy it up, that kind of stuff relaxes me for some reason.
It would mean including pyinspect to the requirements though, @vigji would you be okay with that?

[vigji]

Mmm I would generally oppose aesthetical dependencies, rich is useful for the bar and tables but I would still keep things light - the less moving parts, the better!

[adamltyson]

@FedeClaudi can it be done just with rich?

[FedeClaudi]

it can it can, it'd be just a matter of replicating the functionality provided by the Report class, which is fine

[nickdelgrosso]

Another aspect to address here is that this output is being place in the __repr__ method, which is supposed to be more developer-focused (clear information on how it could be constructed, what the Python type is, structured text, etc). This is a little inconsistent with Python conventions. To have something more readable for the end-user, one would typically use the __str__ method. When both have been implemented, this is what appears when someone types print(atlas); if it hasn't been implemented, then Python will check the __repr__ method. And if you want something that looks nice in the jupyter notebook, that's _repr_html_. A nice overview of these points can be found here: https://dbader.org/blog/python-repr-vs-str

Especially since we're discussing adding more libraries on top of the print, my suggestion would be to implement both the __repr__ and __str__. Something that would produce these two:

>> atlas  # uses __repr__
BrainGlobeAtlas(atlas_name="allen_mouse_25um") 

>> print(atlas)  # uses __str__
allen mouse atlas (res. 25um)
From: http://www.brain-map.org (Wang et al 2020, https://doi.org/10.1016/j.cell.2020.04.007 )

>> str(atlas)  # uses __str__
allen mouse atlas (res. 25um)
From: http://www.brain-map.org (Wang et al 2020, https://doi.org/10.1016/j.cell.2020.04.007 )

[FedeClaudi]

agreed, __repr__ should be more concise and for debugging, while __str__ should print out more detailed info

When I get around doing the fancy metadata printout stuff I will do this as well, thanks Nick

[nickdelgrosso]

So, merge this set of changes, save __repr__ vs __str__ changes for another pull request?

[FedeClaudi]

I can probably just add things here, I will be able to do this later this week.

[FedeClaudi]

Sorry @nickdelgrosso I hadn't noticed that this PR was coming from a forked version of the repo, it might be easier if we merge this and then I'll open a new one for the printing stuff. Is that ok @adamltyson and @vigji ?

[adamltyson]

All good, thanks @FedeClaudi & @nickdelgrosso !

[vigji]

Yup!