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

Refactor utility modules and test_utility.py #350

Closed
5 tasks done
alyssadai opened this issue Sep 28, 2024 · 4 comments · Fixed by #385
Closed
5 tasks done

Refactor utility modules and test_utility.py #350

alyssadai opened this issue Sep 28, 2024 · 4 comments · Fixed by #385
Assignees
Labels
refactor Simplifying or restructuring existing code or documentation. released This issue/pull request has been released.

Comments

@alyssadai
Copy link
Contributor

alyssadai commented Sep 28, 2024

We have a growing number of *_utils.py modules for organizing utility functions shared across or specific to CLI commands. Currently, all these utility modules live in the same top-level directory as the main app (cli.py).

Meanwhile, we have a single test suite for all util functions, which is getting pretty big.

To better organize these util functions and their tests, we should:

  • create a utilities package to store all *_utils.py files
  • rename the shared utility.py to model_utils.py for more specificity
    • also, import as a namespace instead of importing functions directly, for consistency w/ other modules
  • split test_utility.py into util-specific test modules
  • remove test test_generate_context() since it replicates logic of function being tested and is hard to maintain
  • refactor out merging of context + final dataset instance for output .jsonld file, which overlaps across commands (Originally posted by @surchs in [ENH] Add derivatives command and pipeline-catalog submodule #349 (comment))
@alyssadai alyssadai added refactor Simplifying or restructuring existing code or documentation. type:maintenance labels Sep 28, 2024
@alyssadai alyssadai added flag:schedule Flag issue that should go on the roadmap or backlog. and removed flag:schedule Flag issue that should go on the roadmap or backlog. labels Sep 28, 2024
@alyssadai alyssadai moved this to Backlog in Neurobagel Oct 1, 2024
@alyssadai
Copy link
Contributor Author

@surchs, does the issue description make sense to you?

@surchs
Copy link
Contributor

surchs commented Nov 4, 2024

Looks good @alyssadai. So the goal would be to have a single module util to import from, so that any util function could be imported like so from util import my_util_fun, yes?

@alyssadai
Copy link
Contributor Author

alyssadai commented Nov 6, 2024

So the goal would be to have a single module util to import from, so that any util function could be imported like so from util import my_util_fun, yes?

Actually, I would prefer to still import each set of utilities separately, for better organization and clarity on where functions are defined.

At the moment, utility functions live in different .py files based on what CLI command they are used in/relevant for: pheno_utils.py (for functions used by bagel pheno), derivatives_utils.py, etc. And then there are two sets of utilities shared across commands, utility.py (to be renamed model_utils.py for clarity) and file_utils.py.

I would have a directory structure like:

bagel/
   utils/
     __init__.py
     pheno_utils.py
     bids_utils.py
     ...
   ...

and then import them like:

from utils import pheno_utils as putils

since usually all the utilities used by a CLI command are used together. Importing utility functions one at a time would be unnecessarily verbose, I think, and make it harder to see from a given part of code where the function is defined.

@alyssadai alyssadai mentioned this issue Nov 7, 2024
7 tasks
@alyssadai alyssadai moved this from Implement - Active to Implement - Done in Neurobagel Nov 7, 2024
@alyssadai alyssadai self-assigned this Nov 7, 2024
@github-project-automation github-project-automation bot moved this from Review - Active to Review - Done in Neurobagel Nov 11, 2024
Copy link
Contributor

🚀 Issue was released in v0.3.4 🚀

@neurobagel-bot neurobagel-bot bot added the released This issue/pull request has been released. label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Simplifying or restructuring existing code or documentation. released This issue/pull request has been released.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants