Skip to content

[ENH] Update Filename Entity Order - #236

Open
bendhouseart wants to merge 12 commits into
nipreps:mainfrom
bendhouseart:update-entity-order
Open

[ENH] Update Filename Entity Order#236
bendhouseart wants to merge 12 commits into
nipreps:mainfrom
bendhouseart:update-entity-order

Conversation

@bendhouseart

@bendhouseart bendhouseart commented Jan 28, 2026

Copy link
Copy Markdown
Collaborator

Couldn't figure out why changes to niworkflows/data/nipreps.json to address this issue weren't being used here, but got there eventually. Will update the nipreps json at nipreps/niworkflows, but like @effigies or @mgxd mentioned it is probably worth just switching over to the schema instead of relying on these config files.

But I'm also a fan of incremental improvement and small bug fixes.

Changes proposed in this pull request

  • Adds dev container setup based on ghrc.io/nipreps/petprep:main removed and moved to PR [ENH] Add Dev Container #256
  • Updates nipreps.json entity ordering for valid bids output
  • Removes un-used allow_entities

Documentation that should be reviewed

Couldn't come up with a good place to put this in docs so it's self contained in the devcontainer folder:

  • .devcontainer/README.rst

@bendhouseart
bendhouseart requested a review from mgxd January 28, 2026 22:12
@codecov

codecov Bot commented Jan 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.07%. Comparing base (7efcfc0) to head (0a53a09).
⚠️ Report is 206 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #236      +/-   ##
==========================================
+ Coverage   83.06%   83.07%   +0.01%     
==========================================
  Files          87       87              
  Lines        8177     8184       +7     
  Branches      873      874       +1     
==========================================
+ Hits         6792     6799       +7     
  Misses       1163     1163              
  Partials      222      222              

☔ View full report in Codecov by Harness.
📢 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.

@mnoergaard

Copy link
Copy Markdown
Collaborator

Thanks for opening this PR @bendhouseart. However, it is currently mixing several things together, and it would be better have several PRs addressing each issue.

  1. The dev container part is a feature request to extend our debugging options (several already present in the petprep-docker wrapper). Can you please create a new issue regarding this feature request? Then a PR can be opened to address this.

  2. The filename entity order part of this PR addresses several issues ENH: add metadata from raw json data to preproc output jsons #218, Add SkullStripped Boolean to Sidecar Files #229 and Add SpatialReference Information to Outputs #230. Please target each issue separately, as this will make it easier for others review.

Many thanks!

@mnoergaard

Copy link
Copy Markdown
Collaborator

@bendhouseart

1 similar comment
@mnoergaard

Copy link
Copy Markdown
Collaborator

@bendhouseart

Comment thread .gitignore Outdated
@bendhouseart bendhouseart changed the title Update Filename Entity Order; Add Dev Container [ENH] Update Filename Entity Order Mar 23, 2026
@bendhouseart

Copy link
Copy Markdown
Collaborator Author

Suggestions applied; will re-tag for review after CI is passing.

Comment thread petprep/cli/run.py Outdated
@mnoergaard

Copy link
Copy Markdown
Collaborator

@mgxd - do you mind reviewing this? Many thanks

@mgxd mgxd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM - would be nice to have a check outputs step in the CI (like we do in other preps) to ensure produced outputs 1) exist and 2) are named appropriately

@bendhouseart

Copy link
Copy Markdown
Collaborator Author

LGTM - would be nice to have a check outputs step in the CI (like we do in other preps) to ensure produced outputs 1) exist and 2) are named appropriately

You'd need to build and import the schema to the validator for BEP 023, but that's not the worst thing to take care of in CI. It would have the knock on effect of quickly exposing whether the spec or petprep was out of date while outputs are mulled over.

@mgxd

mgxd commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

I meant essentially an outputs manifest (like https://raw.githubusercontent.com/nipreps/fmriprep/refs/heads/master/.circleci/ds005_outputs.txt) to compare to for the time being. Though agreed it would also be nice to validate outputs occasionally

@bendhouseart

Copy link
Copy Markdown
Collaborator Author

I meant essentially an outputs manifest (like https://raw.githubusercontent.com/nipreps/fmriprep/refs/heads/master/.circleci/ds005_outputs.txt) to compare to for the time being. Though agreed it would also be nice to validate outputs occasionally

I'll be happy to review your PR for that ;)

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.

3 participants