Skip to content

Conversation

ubdbra001
Copy link
Contributor

closes #32 (eventually)

I'm happy to leave this open and just add to it as I approach the rest of the methods, but if you'd like to open a PR for each of the rest of the methods then I'm also happy to defer to you on that.

Here's my initial attempt at this:

  • I've developed the low-hanging fruit for Context (path, entities, datatype, suffix, extension, modality, and size) and added stubs for the rest
  • I've also added tests for the methods developed

One thing I wasn't sure about is where it would be beneficial to use cached_property over property for this class (and I suppose more generally). Any advice?

My next step will be to develop the subjects method along with the Sessions class. I think I'll start by creating test for these (so you can make sure I understand how these are meant to work) and then go from there.

@ubdbra001 ubdbra001 changed the title I32 develop context class Develop Context class Sep 8, 2025
Copy link

codecov bot commented Sep 8, 2025

Codecov Report

❌ Patch coverage is 88.60759% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.37%. Comparing base (0a62540) to head (72dbbeb).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/bids_validator/context.py 86.15% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #34      +/-   ##
==========================================
+ Coverage   86.21%   86.37%   +0.15%     
==========================================
  Files          12       12              
  Lines         624      697      +73     
  Branches      103      104       +1     
==========================================
+ Hits          538      602      +64     
- Misses         65       73       +8     
- Partials       21       22       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Cool. A couple quick notes.

@ubdbra001 ubdbra001 force-pushed the i32-develop-context-class branch from 7c10dda to 0d427f4 Compare September 8, 2025 22:55
@ubdbra001 ubdbra001 force-pushed the i32-develop-context-class branch from 9185ba7 to 72dbbeb Compare September 8, 2025 23:12
for Sessions class and associated tests for Context class
@ubdbra001
Copy link
Contributor Author

I just added the tests for the subject method and Sessions class. Do they make sense? Am I missing anything?

@effigies
Copy link
Contributor

effigies commented Sep 9, 2025

Makes sense to me. I would just skip sessions.phenotype. That's been removed in the latest version of BIDS, since it was never used.

@ubdbra001
Copy link
Contributor Author

ubdbra001 commented Sep 9, 2025

Started playing around with this and I'm using the walk_back function to work backwards from the specific file in Context to get the tree for the subject entity.

  1. Does this make sense?
  2. If yes, the walk_back / _walk_back function throws an error because FileParts now expects schema to be passed (Line 249). Shall I just update this so that schema is passed to walk_back? Or do you have a better suggestion?

@effigies
Copy link
Contributor

effigies commented Sep 9, 2025

I would actually probably create subject once when entering a subject directory, and then pass Context(dataset, subject, file) on each file inside it.

def walk(directory, dataset, subject=None):
    if subject is None and is_subject_dir(directory):
        subject = Subject(directory, dataset)

    for child in directory.children:
        yield Context(child, dataset, subject)
        if child.is_dir():
            yield from walk(child, dataset, subject)

tree = FileTree.read_from_filesystem(root)
dataset = Dataset(root, schema)
for context in walk(tree, dataset):
    ...

@ubdbra001
Copy link
Contributor Author

ubdbra001 commented Sep 11, 2025

Right, that does make sense. This should be in __main__.py where we're starting walking through the FileTree from the root directory?
Is this the only part of the context that would benefit from earlier creation it earlier in the walk and then passing it to the Context object?
Looking at the list in #32 I don't think so, but I thought it would be worth double checking.

@ubdbra001
Copy link
Contributor Author

Also, am I right in thinking that we'll still be using the walk_back functions for the sidecar? And if so we'll still need to fix up those functions to pass schema on?

@effigies
Copy link
Contributor

effigies commented Sep 11, 2025

This should be in __main__.py where we're starting walking through the FileTree from the root directory?

Yes, or wherever we do the walk.

Is this the only part of the context that would benefit from earlier creation it earlier in the walk and then passing it to the Context object?

Yes, Dataset and Subject are the only two.

Also, am I right in thinking that we'll still be using the walk_back functions for the sidecar?

Yes.

And if so we'll still need to fix up those functions to pass schema on?

Seems excessive. What do you think about making schema optional and just leaving datatype as None if schema is not passed?

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.

Develop Context class in context.py
2 participants