Skip to content

Loader docs update - #33

Merged
phrgab merged 12 commits into
devfrom
loader-docs-update
May 25, 2026
Merged

Loader docs update#33
phrgab merged 12 commits into
devfrom
loader-docs-update

Conversation

@phrgab

@phrgab phrgab commented May 22, 2026

Copy link
Copy Markdown
Owner

Updating documentation and base loader class docstrings for improved information on implementing new loaders.

As suggested in #29

@phrgab
phrgab requested a review from shum23 May 22, 2026 16:57

@shum23 shum23 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @phrgab. This is great; I've just suggested a couple of tiny wee edits to the docstring example there. Sort of related, I've noticed that hdf5_explorer is available in base_hdf5_class.py but hasn't been exposed at the top level. Do we want to add e.g.

from peaks.core.fileIO.base_data_classes.base_hdf5_class import BaseHDF5DataLoader

hdf5_explorer = BaseHDF5DataLoader.hdf5_explorer

to peaks/__init__.py so that users call it directly as pks.hdf5_explorer(...)

Comment thread peaks/core/fileIO/data_loading.py Outdated
Comment thread peaks/core/fileIO/data_loading.py Outdated
Comment thread peaks/core/fileIO/data_loading.py Outdated
Comment thread peaks/core/fileIO/data_loading.py Outdated
@phrgab

phrgab commented May 25, 2026

Copy link
Copy Markdown
Owner Author

Thanks @shum23 for spotting those syntax errors in the examples - fixed now.

On the question of the hdf5 explorer, I'm a little unsure there. On the one hand, I guess this is mostly a helper function for developers to aid in figuring out the sturcture of the file for building new loaders, and so I wonder if this is a function that will really be used by many users of peaks. On the one hand, I agree it is quite hidden at the moment, and I think I have also used it for "live debugging" of loaders during experiments before.

So, I'm OK to add it to the top level if we think it would be sufficiently useful. Alternatively, we could document it, perhaps in the file loaders explanation section, so it is made more clear. Do you have a view there?

@shum23

shum23 commented May 25, 2026

Copy link
Copy Markdown
Collaborator

Thanks @phrgab. That's fair. I think it's a good idea to add a few words to the explanation section.

Adding tip for hdf5_explorer from BaseHDF5DataLoader class into
developer docs.
Updating hdf5_explorer docstring for consistency
@phrgab

phrgab commented May 25, 2026

Copy link
Copy Markdown
Owner Author

Thanks @shum23, that should be there now

@phrgab
phrgab marked this pull request as ready for review May 25, 2026 14:09

@shum23 shum23 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @phrgab. This is now approved. Please go ahead and merge whenever you're happy with it

@phrgab
phrgab merged commit 1d8db13 into dev May 25, 2026
4 checks passed
@phrgab
phrgab deleted the loader-docs-update branch May 25, 2026 14:25
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.

2 participants