Skip to content

added model selection framework#104

Closed
Devguru-codes wants to merge 3 commits intoOSIPI:mainfrom
Devguru-codes:model-selection-framework
Closed

added model selection framework#104
Devguru-codes wants to merge 3 commits intoOSIPI:mainfrom
Devguru-codes:model-selection-framework

Conversation

@Devguru-codes
Copy link
Copy Markdown
Contributor

@Devguru-codes Devguru-codes commented Mar 8, 2026

I have added ModelSelectionResult dataclass and compare_models() function in file osipy\dce\fitting.py that fits multiple DCE models to the same data and compare them using aic ,bic and R^2.
ModelSelectionResult class holds the best-model map, aic, bic, R^2 score maps, and the full DCEFitResult class which is Result container for DCE model fitting for each model.
compare_models() takes a list of model names, fits each one using the fit_model() pipeline and calculates aic, bic, and return which model is better at each voxel.
3 selection criteria for user - aic, bic and R^2 which is default criterion.
The code also has error handling for edge cases. Every warning is logged and even if 1 fails, other models continues to fit.
Feature fixes #101 is implemented here.
Updated PR also fixes #102

@Devguru-codes
Copy link
Copy Markdown
Contributor Author

Devguru-codes commented Mar 8, 2026

Since the feature implementation and bug fix was in same file so I fixed in same file. @ltorres6 sir please see it. This is just a proof of concept PR, I will update it after your suggestion accordingly. I closed the previous PR as its issue #102 was fixed in this.Thank you sir.

@RajdeepKushwaha5
Copy link
Copy Markdown

Hey! Nice work adding model comparison — that's a useful feature. A few suggestions from reviewing the diff:

1. except Exception: without as e: (issue #102 fix)

The silent pass is replaced with logging, which is great. However the pattern used here:

except Exception:
    logger.debug(...)

doesn't capture the exception object. The rest of the codebase (e.g., bayesian_ivim.py line 206) uses except Exception as e: and logs the actual error:

except Exception as e:
    logger.debug("Voxel %d fitting failed: %s", idx, e)

This gives much better diagnostic information when debugging fitting failures.

2. GPU compatibility in compare_models()

compare_models() uses np.asarray(concentration) directly. The osipy codebase is designed to be GPU/CPU agnostic using xp = get_array_module(array) (see osipy/common/backend/). Hardcoding np.asarray() would break when CuPy or JAX arrays are passed. Consider:

from osipy.common.backend import get_array_module
xp = get_array_module(concentration)

3. Bundling a bug fix (#102) with a feature (#101)

This PR bundles the #102 bug fix (4 lines) with the #101 model-selection feature (200+ lines). Separating them into individual PRs makes review easier and lets the quick bug fix land independently. The contributing guide recommends focused, single-concern PRs.

Hope this helps! Happy to discuss any of these points.

@OSIPI OSIPI deleted a comment from Vedant-Bachhav Mar 22, 2026
@ltorres6
Copy link
Copy Markdown
Collaborator

Closing. See discussion for reasoning.

@ltorres6 ltorres6 closed this Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants