Expand the developer documentation #144

alessandrofelder · 2024-01-29T18:07:23Z

Description

What is this PR

Bug fix
Addition of a new feature
Other

Why is this PR needed?

As a novice (and even as an experienced contributors), it's currently not easy to get a quick, good overview of the how the BrainGlobe code base is organised.

What does this PR do?

This PR represent a major fleshing out of the developer docs.

This PR adds a high-level introduction to the code base for new and existing contributors. It includes:

guiding principles (with explanatory examples)
naming and organisation of packages in org
suggested default software architecture for brainglobe tools

This PR further

incorporates the high-level intro into the wider website
adds some knowledge that was living in our heads to website text, this includes:
- handling of user data (GIN, pooch, .brainglobe folder)
- documentation around test data
- axis ordering docs
- a link to the core development project board
- release process across repos (particularly interested in @willGraham01 looking at this part!)

Note that some of the information about the tool names is slightly ahead of our renaming/merging tool repos schedule 😬 It considers brainglobe/BrainGlobe#43 as already addressed.

References

Closes brainglobe/BrainGlobe#47
Closes #8
Closes #81

brainglobe/BrainGlobe#16 to be tackled separately to this.

How has this PR been tested?

Inspection of local build of website.

Is this a breaking change?

No

Does this PR require an update to the documentation?

This PR is an update to the documentation

Checklist:

The code has been tested locally
[n/a] Tests have been added to cover all new functionality (unit & integration)
The documentation has been updated to reflect any changes
The code has been formatted with pre-commit

intro contains - guiding principles (with explanatory examples) - naming and organisation of packages in org - suggested default software architecture for brainglobe tools Also this commit - incorporates the intro into the wider website - adds some knowledge that was living in our heads to website text.

alessandrofelder · 2024-01-30T18:22:29Z

Asking for feedback from any active developers who have time to look but in particular

@viktorpm because it would be beneficial to get review from someone less familiar with the wider codebase, and because you asked me about this last week 😉
@willGraham01 because you have done the lion's share of recent releases and restructures
@adamltyson because you have the best bird's eye view of the entirety of brainglobe

willGraham01

Most of my comments that aren't in my section are consequences of VSCode religiously using a markdown linter, so apologies for that 😅

One additional bit of info for the releases section - threw in a mention that ideally we follow the package dependency tree when deciding which order to merge any potential PR chains. Just gives some order to the potential merge chaos.

docs/source/community/developers/index.md

docs/source/community/developers/intro_to_codebase.md

docs/source/community/developers/new_releases.md

docs/source/community/developers/testing.md

docs/source/conf.py

docs/source/community/developers/index.md

Co-authored-by: Will Graham <[email protected]>

docs/source/community/developers/intro_to_codebase.md

viktorpm

Thank you @alessandrofelder, this was incredibly useful to read. I added a few comments.

adamltyson

Very nice, thanks @alessandrofelder. I've left a few minor comments.

adamltyson · 2024-02-06T11:35:54Z

docs/source/community/developers/new_releases.md

+- One in the [metapackage repository](https://github.com/brainglobe/brainglobe-meta), which pins the new versions of all affected tools at once.
+
+We should cross-link the latter to the website update, and release all affected packages to PyPI (and conda if appropriate) once they are all merged into `main`.
+Ideally, updates and releases should be made in an order that [follows the dependency tree](./repositories/brainglobe-meta/index#dependency-tree) - starting with our lower level tools, than their dependents, then dependents of those dependents, and so on.


This link is giving me a warning. I don't think sphinx likes section references.

I think it will be happy if you put

./repositories/brainglobe-meta/index.md#dependency-tree

for the link - the .md gets auto-converted to .html so there aren't any problems there. I've used this syntax in a number of places in this repo already without any warnings showing up.

I worked around it differently, for now - but good to know :)

docs/source/community/developers/index.md

adamltyson · 2024-02-06T11:41:58Z

docs/source/community/developers/index.md

+Before contributing code, it may be useful to familiarise yourself with the [introduction to the BrainGlobe code for developers](./intro_to_codebase.md) as well as the [testing](./testing.md), [developer tooling](./tooling.md) and [conventions](./conventions.md) sections. 
+
+The core development team will support you in contributing code, irrespective of your experience.
+At the same time, they will hold you to the same standards as they do each other on review, to ensure BrainGlobe remains easy-to-maintain. 


Can we make this more friendly? I would suggest that we hold all contributions to the same standard, but not necessarily all developers (i.e. we may go in and fix something ourselves).

Suggested change

At the same time, they will hold you to the same standards as they do each other on review, to ensure BrainGlobe remains easy-to-maintain.

At the same time, they will hold you to the same standards as they do each other on review, to ensure BrainGlobe remains easy-to-maintain.

docs/source/community/developers/index.md

docs/source/community/developers/new_releases.md

adamltyson · 2024-02-06T11:53:56Z

docs/source/community/developers/intro_to_codebase.md

+* ease of installation: through the metapackage, we provide a one-line command to install all BrainGlobe tools at once. None of the packages depend on anything other than Python (we've removed historical compiled code), and are therefore easy to install cross-platform.
+* accessibility: we aim to provide a Graphical User Interface (GUI) for all BrainGlobe tools. By asking users for feedback, we ensure that the GUI provides a nice user experience.
+* ease of use through Python/interoperability: we aim to provide a well-documented Python API for all BrainGlobe tools.
+* performance: By running weekly benchmarks comparing the latest release with the development version, we guarantee that performance will not deteriorate as BrainGlobe evolves.
+* species/modality independent: none of the code makes any assumptions about the imaging modality or the species of the model organism of interest. We provide atlases for a variety of model organisms.
+* useability: we sacrifice the code simplicity provided by `magicgui` in exchange for fine-grained control of the user experience by writing brainglobe widgets in `qtpy`. This is an example where the first guiding principle takes priority over the second.
+* easy-to-maintain: we move functionality used by more than one independent BrainGlobe tool to `brainglobe-utils` to reduce code duplication and make maintenance easier.


Is there a reason all of these bullets aren't capitalised?

adamltyson · 2024-02-06T11:56:12Z

docs/source/community/developers/intro_to_codebase.md

+- [`brainglobe-registration`](https://github.com/brainglobe/brainglobe-registration)
+- [`brainrender-napari`](https://github.com/brainglobe/brainrender-napari)
+
+The BrainGlobe Github organisation also hosts the [`brainglobe` (meta-)package](./repositories/brainglobe-meta/index.md) and the [`brainglobe-workflows` collection](./repositories/brainglobe-workflows/index.md) in separate repositories (which are not tools in themselves), as well as the utility packages `brainglobe-utils` and `brainglobe-space`.


I would argue that brainglobe-space is a tool. I've used it independently of the BG ecosystem before.

adamltyson · 2024-02-06T11:57:18Z

docs/source/community/developers/intro_to_codebase.md

+User data is kept in hidden folders, usually in the user's `$HOME` directory.
+These folders are named after the tools (e.g. `~/.cellfinder/`) or in appropriate subfolders of `.brainglobe` (e.g. `~/.brainglobe/mpin_zfish_1um_v1.0/`).


Can we add that although this is the current status, all new locally stored files should go into ~.brainglobe?

adamltyson · 2024-02-06T11:58:01Z

docs/source/community/developers/intro_to_codebase.md

+
+### Default architecture for BrainGlobe Tools
+
+By default, each BrainGlobe tool should be organised into three distinct submodules: `core`, `qt`, and `napari`.


Maybe specify up to three? And we won't have this structure if there's no GUI.

adamltyson · 2024-02-06T11:58:29Z

docs/source/community/developers/intro_to_codebase.md

+- the modularity ensure each of submodule can (and should be) tested individually in the first instance
+- widgets can be re-used outside of napari
+- integration tests in the same repo to avoid messy CI dependencies


Capitalisation?

Co-authored-by: Adam Tyson <[email protected]>

alessandrofelder added 8 commits January 29, 2024 18:05

minor tweaks

c2fcec6

ignore sign-in only link

98d8d80

document axis ordering conventions

3df7f97

link to core dev project board

daff24f

document release process and frequency

436f76c

document handling of test data and user folders

bfdc133

link to GH for tools

44c1f47

alessandrofelder requested review from a team, adamltyson and willGraham01 January 30, 2024 18:16

alessandrofelder marked this pull request as ready for review January 30, 2024 18:17

make linkcheck happy

1aa5782

alessandrofelder requested a review from viktorpm January 30, 2024 18:18

alessandrofelder changed the title ~~Write high-level intro to codebase~~ Expand the developer documentation Jan 30, 2024

willGraham01 reviewed Jan 31, 2024

View reviewed changes

viktorpm reviewed Jan 31, 2024

View reviewed changes

docs/source/community/developers/index.md Outdated Show resolved Hide resolved

Apply suggestions from @willGraham01's code review

f7158a8

Co-authored-by: Will Graham <[email protected]>

viktorpm reviewed Jan 31, 2024

View reviewed changes

docs/source/community/developers/intro_to_codebase.md Outdated Show resolved Hide resolved

alessandrofelder added 2 commits January 31, 2024 10:39

Apply suggestions from @viktorpm 's code review

d86f645

fix typo

38a8ecf

viktorpm approved these changes Jan 31, 2024

View reviewed changes

adamltyson approved these changes Feb 6, 2024

View reviewed changes

alessandrofelder and others added 4 commits February 7, 2024 15:56

Apply suggestions from code review

e81833d

Co-authored-by: Adam Tyson <[email protected]>

minor doc tweaks following review

8703e45

avoid direct section ref

5255b78

small reword of user data section

fa6ee99

alessandrofelder merged commit e6f5ba0 into main Feb 7, 2024
2 checks passed

alessandrofelder deleted the developer-docs branch February 7, 2024 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand the developer documentation #144

Expand the developer documentation #144

alessandrofelder commented Jan 29, 2024 •

edited

Loading

alessandrofelder commented Jan 30, 2024

willGraham01 left a comment

viktorpm left a comment

adamltyson left a comment

adamltyson Feb 6, 2024

willGraham01 Feb 9, 2024

alessandrofelder Feb 9, 2024

adamltyson Feb 6, 2024

adamltyson Feb 6, 2024

adamltyson Feb 6, 2024

adamltyson Feb 6, 2024

adamltyson Feb 6, 2024

adamltyson Feb 6, 2024

	At the same time, they will hold you to the same standards as they do each other on review, to ensure BrainGlobe remains easy-to-maintain.
	At the same time, they will hold you to the same standards as they do each other on review, to ensure BrainGlobe remains easy-to-maintain.

		User data is kept in hidden folders, usually in the user's `$HOME` directory.
		These folders are named after the tools (e.g. `~/.cellfinder/`) or in appropriate subfolders of `.brainglobe` (e.g. `~/.brainglobe/mpin_zfish_1um_v1.0/`).


		### Default architecture for BrainGlobe Tools

		By default, each BrainGlobe tool should be organised into three distinct submodules: `core`, `qt`, and `napari`.

Expand the developer documentation #144

Expand the developer documentation #144

Conversation

alessandrofelder commented Jan 29, 2024 • edited Loading

Description

References

How has this PR been tested?

Is this a breaking change?

Does this PR require an update to the documentation?

Checklist:

alessandrofelder commented Jan 30, 2024

willGraham01 left a comment

Choose a reason for hiding this comment

viktorpm left a comment

Choose a reason for hiding this comment

adamltyson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandrofelder commented Jan 29, 2024 •

edited

Loading