Skip to content

230 add tabluar output from solution analysis #250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 13 commits into from
Apr 22, 2025
Merged

230 add tabluar output from solution analysis #250

merged 13 commits into from
Apr 22, 2025

Conversation

@peterhollender
Copy link
Contributor

@peterhollender peterhollender commented Mar 18, 2025 •
edited
Loading

Closes #230

@peterhollender peterhollender linked an issue Mar 18, 2025 that may be closed by this pull request
Add tabluar output from solution analysis #230
Closed
@peterhollender
Copy link
Contributor Author

peterhollender commented Mar 18, 2025

I feel like SolutionAnalysis should probably get rewritten at some point so that instead of a bunch of loose attributes + a parameter_constraints dict + hard coded formatting info, it has a dictionary of SolutionAnalysisAttribute objects, each of which contains the information needed to apply the constraints, format display, etc. Then we could have @property methods for convenient access, fancier retrieval of values (specify alternative output units for example), etc. But this is working, so we can handle that later

@ebrahimebrahim
Copy link
Collaborator

ebrahimebrahim commented Mar 18, 2025

Agreed about your SolutionAnalysisAttribute proposal.

Once this is ready you can tag @arhowe00 for review please, since he will work on OpenwaterHealth/SlicerOpenLIFU#98

(feel free to tag for review without fixing the units tests or whatever is failing, we can look at that after and handle it)

We have the tabular output working in SlicerOpenLIFU by the way (in one of Andrew's topic branches I think), but it shows raw floats with full precision, so this will help to format things!

@ebrahimebrahim
Copy link
Collaborator

ebrahimebrahim commented Mar 18, 2025

Oh I wrote that before realizing you've already tagged me to review. Bringing in Andrew 😄

@ebrahimebrahim ebrahimebrahim requested a review from arhowe00 March 18, 2025 20:46
@arhowe00 arhowe00 force-pushed the 230-add-tabluar-output-from-solution-analysis branch from 192a918 to b8826e0 Compare March 25, 2025 18:41
@arhowe00
Copy link
Contributor

arhowe00 commented Mar 25, 2025 •
edited
Loading

@peterhollender FYI I rebased to main because there were some merge conflicts. Still working on review

@arhowe00 arhowe00 force-pushed the 230-add-tabluar-output-from-solution-analysis branch from b8826e0 to e9a5351 Compare March 25, 2025 18:53
@arhowe00 arhowe00 mentioned this pull request Mar 26, 2025
Closed
@arhowe00 arhowe00 force-pushed the 230-add-tabluar-output-from-solution-analysis branch 2 times, most recently from d8f8ed1 to 14c3128 Compare April 3, 2025 18:34
arhowe00
arhowe00 requested changes Apr 3, 2025
View reviewed changes
Copy link
Contributor

@arhowe00 arhowe00 left a comment

Choose a reason for hiding this comment

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

I rebased this to the main branch and looked at the changes. For the most part, things make sense, there were some changes I made to satisfy the linter (some of these were more important than others, like passing a list to a tuple). There were some things that needed imports, such as PARAM_STATUS_SYMBOLS, etc.

If you want @peterhollender I can apply most of these changes in batch, just reply to what you think about each of them if you have any points of contention and I'll decide what to commit based on that.

@ebrahimebrahim ebrahimebrahim mentioned this pull request Apr 10, 2025
Merged
arhowe00 pushed a commit that referenced this pull request Apr 14, 2025
@peterhollender @arhowe00
Address comments (#230)
8a91d1f 
Addresses comments on [SlicerOpenLIFU PR#250
review](#250 (review))
@arhowe00 arhowe00 force-pushed the 230-add-tabluar-output-from-solution-analysis branch from c7de2ed to 579bf96 Compare April 14, 2025 17:45
arhowe00 added a commit that referenced this pull request Apr 14, 2025
@arhowe00
Add review suggestions (#230)
c4364fe 
Adds suggestions made in [a review of SlicerOpenLIFU
PR#250](#250 (review))
@arhowe00 arhowe00 force-pushed the 230-add-tabluar-output-from-solution-analysis branch from c4364fe to 7aeeee5 Compare April 14, 2025 20:38
arhowe00 pushed a commit that referenced this pull request Apr 14, 2025
@peterhollender @arhowe00
Address comments (#230)
dd5fdac 
Addresses comments on [SlicerOpenLIFU PR#250
review](#250 (review))
arhowe00 added a commit that referenced this pull request Apr 14, 2025
@arhowe00
Add review suggestions (#230)
7aeeee5 
Adds suggestions made in [a review of SlicerOpenLIFU
PR#250](#250 (review))
arhowe00 pushed a commit that referenced this pull request Apr 14, 2025
@peterhollender @arhowe00
Address comments (#230)
9454dea 
Addresses comments on [SlicerOpenLIFU PR#250
review](#250 (review))
arhowe00 added a commit that referenced this pull request Apr 14, 2025
@arhowe00
Add review suggestions (#230)
a66abcc 
Adds suggestions made in [a review of SlicerOpenLIFU
PR#250](#250 (review))
@arhowe00 arhowe00 force-pushed the 230-add-tabluar-output-from-solution-analysis branch from 7aeeee5 to a66abcc Compare April 14, 2025 20:50
arhowe00
arhowe00 requested changes Apr 14, 2025
View reviewed changes
Copy link
Contributor

@arhowe00 arhowe00 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is close to being merged, I just rebased again to main and still have a comment below that needs to be addressed.

Edit: This comment no longer needs to be addressed.

@arhowe00
Copy link
Contributor

arhowe00 commented Apr 21, 2025 •
edited
Loading

I'll rebase and merge this afternoon.

@peterhollender @arhowe00
Accept auto fixes (#230)
6430c5f 
Refactored pressure unit from Pa to MPa; added to_table method for
tabular output; introduced ParameterConstraint class for status
evaluation; updated test data accordingly.
@peterhollender @arhowe00
Fix keys (#230)
d3880e9 
- Introduced ParameterConstraint class for validating and evaluating
  analysis parameters.
- Integrated constraint handling into Protocol.calc_solution and
  Solution.analyze.
- Updated SolutionAnalysis.to_table to include constraint status.
- Modified example_protocol.json to define parameter constraints.
- Added param_constraint.py and corresponding test notebook.
@peterhollender @arhowe00
Fixed CI issues (#230)
1aabdbe 
Added from __future__ import annotations across modules to resolve CI
issues; updated type annotations for ParameterConstraint references to
eliminate forward declarations.
@peterhollender @arhowe00
Remove is_sorted (#230)
b78e566 
Replaced is_sorted checks with explicit tuple order comparisons; removed
unused import from more_itertools.
peterhollender and others added 9 commits April 22, 2025 00:44
@peterhollender @arhowe00
Update solution analysis test (#230)
210456c 
 - Added check for param_constraints being a dict
 - Adjusted type assertions for other fields in
   test_solution_analyze_data_types
@peterhollender @arhowe00
Accept autofix (#230)
4bb692f 
Replace default None with field(default_factory=dict) for
param_constraints in Solution.analyze; formatting-only change in
test_solution.py.
@peterhollender @arhowe00
Fix use of field (#230)
b991962 
Replaces field(default_factory=dict) with plain {} as default for
param_constraints in Solution.analyze.
@peterhollender @arhowe00
Fixed unit tests for required dims (#230)
dfa182a 
Standardized coordinate dimension names (x, y, z → lat, ele, ax);
adjusted test inputs and protocol serialization; changed MI calculation
to use max value; updated parameter formatting for MI.
@peterhollender @arhowe00
Added focus_index to analysis.to_table (#230)
abada81 
 - Add optional focus_index param to SolutionAnalysis.to_table to select
   value per focus
 - Updated logic to handle per-focus values vs aggregated
 - Modified formatting and constraint evaluation accordingly
 - Adjusted test notebook to use focus_index with constraints
@arhowe00
Update db_dvc.dvc (#230)
daec8d2 
Update db_dvc.dvc to contain the updated class fields by running the
command at the state of the current revision:

```
python scripts/standardize_database.py db_dvc
```
@peterhollender @arhowe00
Address comments (#230)
57ed185 
Addresses comments on [SlicerOpenLIFU PR#250
review](#250 (review))
@peterhollender @arhowe00
Add type hints to gen_matrix_array (#230)
80ef70b
@arhowe00
Add review suggestions (#230)
4b52c6f 
Adds suggestions made in [a review of SlicerOpenLIFU
PR#250](#250 (review))
@arhowe00 arhowe00 force-pushed the 230-add-tabluar-output-from-solution-analysis branch from a66abcc to 4b52c6f Compare April 22, 2025 04:45
@arhowe00 arhowe00 merged commit 9b26f46 into main Apr 22, 2025
9 checks passed
arhowe00 pushed a commit that referenced this pull request Apr 22, 2025
@peterhollender @arhowe00
Address comments (#230)
a5ae628 
Addresses comments on [SlicerOpenLIFU PR#250
review](#250 (review))
@arhowe00 arhowe00 mentioned this pull request Apr 22, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arhowe00 arhowe00 arhowe00 approved these changes

@ebrahimebrahim ebrahimebrahim Awaiting requested review from ebrahimebrahim

Assignees

@arhowe00 arhowe00

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add tabluar output from solution analysis

4 participants

@peterhollender @ebrahimebrahim @arhowe00