Skip to content

Conversation

@MaximeBICMTL
Copy link
Contributor

@MaximeBICMTL MaximeBICMTL commented Oct 30, 2025

This PR batches several of my PRs for easier testing and rebasing. The code of all the PRs included in this branch have been reviewed and approved by @jeffersoncasimir.

DO NOT SQUASH WHEN MERGING so that each individual PR appears in the git history.

@MaximeBICMTL MaximeBICMTL changed the title Maxime changes Batch Maxime PRs Oct 30, 2025
@maximemulder maximemulder reopened this Oct 30, 2025
@MaximeBICMTL MaximeBICMTL marked this pull request as ready for review October 30, 2025 16:12
Copy link
Contributor

@jeffersoncasimir jeffersoncasimir left a comment

Choose a reason for hiding this comment

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

Approving this PR which is combination of PRs I previously approved individually.

I successfully ran the current aces/main bids_import.py pipeline with these changes.

The test_orm_sql_sync.py integration test can remove a lot of doubt that might surround the robustness of the ORM transition.

The majority of the rest of the python changes are minor changes regarding standardizing the license and referencing the new config location. For perl it's similar, but regarding the config file reference and enforcing whitespace consistency.

I think this is ready to be merged and I will be testing the Incremental BIDS Importer PR next, which is rebased on this one and now much more digestible

@cmadjar
Copy link
Collaborator

cmadjar commented Nov 5, 2025

@jeffersoncasimir @MaximeBICMTL Two questions coming from testing the BIDS import on CBIGR today, not sure if that has implications on the PRs sent to this repo:

  • even if the bids validation is turned off, it requires dataset_description.json in the BIDS dataset to import. Is that expected?
  • got an error that -b option does not exist when using -b short cut option. We had to use --nobidsvalidation long option instead
  • the creation of session via the BIDS importer did not work (@jeffersoncasimir did you try the creation of visits and candidates when testing the PR?)

Maybe those are just happening on CBIGR but just in case, figured I would mention it here.

@MaximeBICMTL
Copy link
Contributor Author

MaximeBICMTL commented Nov 6, 2025

Hi @cmadjar, thank you for your feedback on the BIDS importer!

Though note that this PR does not contain changes to the BIDS importer, the BIDS importer used in C-BIG and that I want to merge here is #1325. This PR only contains general changes that do not affect the behavior of individual scripts.

I answered your points in the relevant BIDS importer PR.

@cmadjar cmadjar merged commit 58f9bed into main Nov 6, 2025
9 of 10 checks passed
@cmadjar cmadjar added this to the 28.0.0 milestone Nov 19, 2025
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.

5 participants