Skip to content

Future proofing - #1589

Merged
KrisThielemans merged 6 commits into
UCL:masterfrom
KrisThielemans:future_proofing
Apr 16, 2025
Merged

Future proofing#1589
KrisThielemans merged 6 commits into
UCL:masterfrom
KrisThielemans:future_proofing

Conversation

@KrisThielemans

@KrisThielemans KrisThielemans commented Apr 13, 2025

Copy link
Copy Markdown
Collaborator

prepare for possible changes in future to enable GPU array etc. This might mean stir::Array template arguments change in the future, or DiscretisedDensity is derived from a different class. It is now recommended to

  • include stir/ArrayFwd.h when using forward declaration
  • use the ArrayType template-alias in places where a rectangular array that might live on a GPU is intended.

@KrisThielemans
introduce ArrayFwd.h for forward declaration of Array
35cbf86 
As stir:Array template arguments might change in the future,
it is now recommended to include ArrayFwd.h when using forward
declaration.
@KrisThielemans
introduce ArrayType template-alias for future proofing
07d968d
@KrisThielemans
update release notes
ce99e2c
@KrisThielemans KrisThielemans added this to the v6.3 milestone Apr 13, 2025
@KrisThielemans
KrisThielemans requested a review from NikEfth April 13, 2025 08:48
@KrisThielemans KrisThielemans self-assigned this Apr 13, 2025
@KrisThielemans KrisThielemans mentioned this pull request Apr 13, 2025
Open
@KrisThielemans
correct typedef
909688f 
This caused problems for SWIG
@NikEfth

NikEfth commented Apr 13, 2025

Copy link
Copy Markdown
Collaborator

How should I go about that?
Several options I merge this to my branch, do anything that is needed and commit those changes and pr here. Then you merge to master and I sync.

It should be able to avoid the conflicts, right?

@KrisThielemans

KrisThielemans commented Apr 13, 2025

Copy link
Copy Markdown
Collaborator Author

No. Anything in your branch will contaminate. Just have a look here if I missed anything obvious. We merge to master. You merge master on yours and just fix whatever.

@KrisThielemans

KrisThielemans commented Apr 14, 2025

Copy link
Copy Markdown
Collaborator Author

Python tests still failing with

 AttributeError: 'stir.FloatVoxelsOnCartesianGrid' object has no attribute 'fill'

Seems that SWIG doesn't understand the way we use using

@NikEfth

NikEfth commented Apr 14, 2025

Copy link
Copy Markdown
Collaborator

I think it's ok, but:
But, I am a bit worried about the IO functions as they are very specific to the current Array and ArrayType hides the type of Array you are using. You will have to go through the existing read_ & write_

@KrisThielemans

KrisThielemans commented Apr 14, 2025

Copy link
Copy Markdown
Collaborator Author

Of course, you can change the read_from_file etc in your PR to do the proper thing for the new type. It's a bit hard for me to see how I'd do this generically. Or are you saying I should undo the changes for read_from_file etc?

@KrisThielemans
KrisThielemans merged commit 4e57186 into UCL:master Apr 16, 2025
@KrisThielemans

KrisThielemans commented Apr 16, 2025

Copy link
Copy Markdown
Collaborator Author

@NikEfth I thought it best to merge this now. Please merge onto your PR and adjust at will.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NikEfth NikEfth Awaiting requested review from NikEfth

Assignees

@KrisThielemans KrisThielemans

Labels

enhancement

Projects

None yet

Milestone

v6.3

Development

Successfully merging this pull request may close these issues.

2 participants

@KrisThielemans @NikEfth