Implement checks for fulfilment of algorithm requirements in the OsipiBase class by harshithasudhakar · Pull Request #96 · OSIPI/TF2.4_IVIM-MRI_CodeCollection

harshithasudhakar · 2025-03-20T21:38:04Z

Describe the changes you have made in this PR

Implemented a centralized validation logic for inputs in the base class.

Link this PR to an issue [optional]

Fixes#45

Checklist

Self-review of changed code
Added automated tests where applicable
Update Docs & Guides

etpeterson · 2025-03-21T04:39:47Z

Thanks for this. It's a good start. I'd like some feedback from @IvanARashid on this too. Basically I have one main suggestion for making it easier to maintain. It would also be great to get a test that goes through the algorithms and somehow verifies the validators are running.

etpeterson · 2025-03-21T04:45:56Z

Also looks like tests are failing because of this. I think it's too strict, or not all the algorithms are acting in quite the standard way.

IvanARashid · 2025-03-21T13:20:04Z

Would be good if the check error messages also prints both the requirement and the current input. It would make it clearer for us why the tests are currently failing

harshithasudhakar · 2025-03-29T11:41:53Z

Sorry for the delay, I've made the changes now. Please let me know if there are any more changes I should make.

etpeterson · 2025-03-30T22:26:04Z

Still failing tests.

etpeterson

Looking pretty good but the tests are still failing as well.

etpeterson

The tests are still failing because of the checks you added. For example here: https://github.com/OSIPI/TF2.4_IVIM-MRI_CodeCollection/actions/runs/14249360771/job/39959244676#step:6:79
Either the checks need to be modified or how the algorithms are called. Probably the algorithm calls aren't consistent or as they should be, but I haven't looked into it. Let me know if you need some help with this.

etpeterson · 2025-04-20T20:07:02Z

Checking in on this because someone might start work on tests related to this.

etpeterson · 2025-04-25T13:30:31Z

Hi @harshithasudhakar, thanks for this. I think the major remaining issue is the failing tests. Are you seeing these?

etpeterson

The failing tests need to be resolved before merging.

harshithasudhakar · 2025-05-01T11:40:06Z

Hi @harshithasudhakar, thanks for this. I think the major remaining issue is the failing tests. Are you seeing these?

I've fixed them, the test cases should pass now.

etpeterson

Tests are still failing, sorry.

etpeterson · 2025-05-27T03:40:55Z

Just checking on this, are you planning on finishing this up?

oliverchampion

Hey! I was thinking it may make more sense to come up with some "default" values for initial guess, bounds and cut-off when they are missing, rather then give an error.

Now, I personally solve the missing of these values in my wrapper. But it may be better to have the Osipi base to come up with default values that are identical for all algorithms.

oliverchampion · 2025-06-24T08:14:04Z

- #else: 
- #return True
- return True
+ if not hasattr(self, "required_thresholds"):


I am wondering whether for some attributes, like "required_thresholds", we should add "default" values when tests fail. Like, 200 would be an okay value for most body applications. Then we can do with a warning stating that the default value is used, instead.

oliverchampion · 2025-06-24T08:14:46Z

- #else:
- #return True
- return True
+ if self.required_bounds is False and self.bounds is None:


Same for bounds. I think if no bounds are given, but bounds will be used, then we should give some default bounds.

oliverchampion · 2025-06-24T08:15:04Z

- def osipi_check_required_bvalues():
- """Minimum number of b-values required"""
- pass
+ if self.required_initial_guess is False and self.initial_guess is None:


same for initial guess :)

oliverchampion · 2025-06-24T08:15:36Z

+ f"Requires {min_dim}-{max_dim}D."
+ )

 def osipi_check_required_bvalues(self):


b-values is another story. Here we cannot give a "default"

oliverchampion · 2026-01-20T09:07:05Z

what is the status of this PR? should we merge/close?

harshithasudhakar · 2026-01-21T20:02:22Z

Hey! I was thinking it may make more sense to come up with some "default" values for initial guess, bounds and cut-off when they are missing, rather then give an error.
Now, I personally solve the missing of these values in my wrapper. But it may be better to have the Osipi base to come up with default values that are identical for all algorithms.

@oliverchampion Well, some algorithms don't require all parameters. So wouldn't it be best to leave it as None ?

oliverchampion · 2026-05-05T09:20:44Z

Not sure if all of this is still equally relevant, but having tests for default values is relevant.
@IvanARashid, I think you have most insight in the wrapper. Can you check whether all these tests are still relevant, also after the recent push by devguru?

oliverchampion · 2026-05-29T09:11:05Z

@ericpete @harshithasudhakar , sorry for late reactions. I'm more active now :).
Is this PR still relevant? If yes, please resolve the conflicts and previous comment. If not relevant, I will close it over the next weeks :)

harshithasudhakarand others added 2 commits March 14, 2025 02:27

Ignore virtual environment folder
f509b5e

implemented validation functions
542a272

harshithasudhakar changed the title ~~implemented validation functions~~Implement checks for fulfilment of algorithm requirements in the OsipiBase classMar 20, 2025

harshithasudhakar added 3 commits March 21, 2025 03:18

added validation checks in fit() and fit_full_volume()
38a2e72

updated subclasses to implement validation check
91ead9d

Merge branch 'main' into main
3cbd154

harshithasudhakar marked this pull request as ready for review March 20, 2025 22:31

etpeterson requested changes Mar 21, 2025
View reviewed changes

Comment thread.gitignore Outdated
Comment threadsrc/standardized wip/IAR_LU_linear.py Outdated

harshithasudhakar added 2 commits March 29, 2025 17:07

centralized input validation
e550f80

removed local changes
4739089

harshithasudhakar requested review from IvanARashid and etpeterson March 29, 2025 11:43

etpeterson requested changes Mar 30, 2025
View reviewed changes

Comment threadsrc/standardized/ETP_SRI_LinearFitting.py Outdated
Comment threadsrc/wrappers/OsipiBase.py Outdated
Comment threadsrc/wrappers/OsipiBase.py

harshithasudhakar added 2 commits March 31, 2025 17:53

remove duplicated call
953e872

lenient check in __init__
d49d63f

harshithasudhakar requested a review from etpeterson April 1, 2025 20:45

etpeterson requested changes Apr 2, 2025
View reviewed changes

Comment threadsrc/wrappers/OsipiBase.py Outdated

remove return statements
04058d1

etpeterson requested changes Apr 4, 2025
View reviewed changes

etpeterson mentioned this pull request Apr 20, 2025
[Feature] <Test the wrapper alone> #85
Open

harshithasudhakar added 2 commits April 24, 2025 09:45

store data before validation
a2d8a22

update fit_full_volume
1dd548e

etpeterson requested changes Apr 25, 2025
View reviewed changes

harshithasudhakar added 2 commits April 30, 2025 15:55

modify algos to use a tuple for accepted_dimensions
e399f31

exclude bvalue axis
e20a8e2

etpeterson requested changes May 9, 2025
View reviewed changes

oliverchampion reviewed Jun 24, 2025
View reviewed changes

harshithasudhakar added 2 commits January 22, 2026 00:32

Merge branch 'main' into main
f05032b

fixes
7e58f2f

harshithasudhakar requested review from etpeterson and oliverchampion January 21, 2026 20:03

Uh oh!

Conversation

harshithasudhakar commented Mar 20, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the changes you have made in this PR

Link this PR to an issue [optional]

Checklist

Uh oh!

etpeterson commented Mar 21, 2025

Uh oh!

Uh oh!

Uh oh!

etpeterson commented Mar 21, 2025

Uh oh!

IvanARashid commented Mar 21, 2025

Uh oh!

harshithasudhakar commented Mar 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etpeterson commented Mar 30, 2025

Uh oh!

etpeterson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

etpeterson left a comment

Choose a reason for hiding this comment

Uh oh!

etpeterson commented Apr 20, 2025

Uh oh!

etpeterson commented Apr 25, 2025

Uh oh!

etpeterson left a comment

Choose a reason for hiding this comment

Uh oh!

harshithasudhakar commented May 1, 2025

Uh oh!

etpeterson left a comment

Choose a reason for hiding this comment

Uh oh!

etpeterson commented May 27, 2025

Uh oh!

oliverchampion left a comment

Choose a reason for hiding this comment

Uh oh!

oliverchampionJun 24, 2025

Choose a reason for hiding this comment

Uh oh!

oliverchampionJun 24, 2025

Choose a reason for hiding this comment

Uh oh!

oliverchampionJun 24, 2025

Choose a reason for hiding this comment

Uh oh!

oliverchampionJun 24, 2025

Choose a reason for hiding this comment

Uh oh!

oliverchampion commented Jan 20, 2026

Uh oh!

harshithasudhakar commented Jan 21, 2026

Uh oh!

oliverchampion commented May 5, 2026

Uh oh!

oliverchampion commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

harshithasudhakar commented Mar 20, 2025•
edited
Loading

etpeterson left a comment •
edited
Loading