Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a small set of core quality-of-life updates across POI computation/logging, NIfTI wrapper ergonomics, BIDS identifier generation, and a few typing/constants tweaks.
Changes:
- Add/propagate a
verboseflag through non-centroid POI computation and adjust default verbosity incalc_poi_from_subreg_vert. - Refine POI sorting behavior to avoid overriding a provided
order_dictwhenlevel_one_infois unset (Any). - Add convenience setters to
NIIproperties (dtype,orientation,zoom), extract BIDS identifier creation intoBIDS_FILE, and tweak typing/constants.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
TPTBox/core/poi_fun/vertebra_pois_non_centroids.py |
Adds verbose plumbing to POI computation log messages. |
TPTBox/core/poi_fun/poi_abstract.py |
Adjusts sort() to not force order_dict when level_one_info is Any. |
TPTBox/core/poi.py |
Changes default verbosity, types _vert_ids, and forwards verbose into non-centroid POI computation. |
TPTBox/core/np_utils.py |
Updates the annotated return type of np_calc_crop_around_centerpoint. |
TPTBox/core/nii_wrapper.py |
Adds setters for dtype, orientation, and zoom. |
TPTBox/core/nii_poi_abstract.py |
Changes how assert_affine() prints failures (now aggregates into text). |
TPTBox/core/bids_files.py |
Adds BIDS_FILE.get_identifier() and reuses it from BIDS_Family.get_identifier(). |
TPTBox/core/bids_constants.py |
Tweaks the formats list entry around "MT"/"MTS". |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cutout_size: tuple[int, ...], | ||
| pad_to_size: Sequence[int] | np.ndarray | int = 0, | ||
| ) -> tuple[np.ndarray, tuple, tuple]: | ||
| ) -> tuple[np.ndarray, tuple[slice, slice, slice], tuple]: |
There was a problem hiding this comment.
The return type annotation is now fixed to exactly three slices, but this function is n-dimensional (it builds cutout_coords_slices with length n_dim). This annotation will be incorrect for non-3D inputs and will break static typing for 2D/4D use. Consider changing the type to tuple[slice, ...] (or tuple[slice, ...] parameterized by n_dim) to match the implementation.
| for err in found_errors: | ||
| log.print(err, ltype=Log_Type.FAIL, verbose=verbose) | ||
| text = f"{text}; {err}" if text else f"{err}" | ||
| log.print(f"{text}", ltype=Log_Type.FAIL, verbose=verbose) |
There was a problem hiding this comment.
text is being mutated inside the loop and then printed each iteration, which causes later log lines to include earlier errors (e.g., second line becomes "err1; err2"). This produces duplicated/expanding messages and makes the per-error output hard to read. Consider either printing each err with the original text prefix (without mutating it), or build a single aggregated message once after the loop and print/raise that.
| # if "sub" not in first_e.info: | ||
| # print(f"family_id, no sub-key, got {first_e.info} and data_dict {list(self.data_dict.keys())}") | ||
| # identifier = "sub-404" | ||
| # else: | ||
| # identifier = "sub-" + first_e.info["sub"] | ||
| # for s in first_e.info.keys(): | ||
| # if s in self.sequence_splitting_keys: | ||
| # identifier += "_" + s + "-" + first_e.info[s] | ||
| # return identifier |
There was a problem hiding this comment.
There is a large block of commented-out legacy implementation left after the new early return. Since it is now unreachable and duplicates the extracted logic, it should be removed to avoid confusion and reduce maintenance overhead.
| # if "sub" not in first_e.info: | |
| # print(f"family_id, no sub-key, got {first_e.info} and data_dict {list(self.data_dict.keys())}") | |
| # identifier = "sub-404" | |
| # else: | |
| # identifier = "sub-" + first_e.info["sub"] | |
| # for s in first_e.info.keys(): | |
| # if s in self.sequence_splitting_keys: | |
| # identifier += "_" + s + "-" + first_e.info[s] | |
| # return identifier |
No description provided.