Skip to content
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.

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

python plan interface: modify attributes casues segfault #565

Open
fzimmermann89 opened this issue Sep 23, 2024 · 2 comments
Open

python plan interface: modify attributes casues segfault #565

fzimmermann89 opened this issue Sep 23, 2024 · 2 comments
Assignees

Comments

@fzimmermann89
Copy link

Hi,

using the python Plan interface it is possible to cause a python crash by, for example, creating a plan with n_trans=2, then modifying n_trrans=1 and executing the plan (with n_trans=1 data)

Maybe these properties should be read-only properties of the plan, or modifying them should invalidate the underlying plan and create a new one on execute.

Background: In our use case, everything stays the same between different invocations of the nufft except the batch size. We thought we could change this attribute of the plan and reuse it. Instead, we now need to check if it changed and invalidate the plan if it did.

For us, it would be great if modifying the attributes of the python object would invalidate the underlying c-plan, execute would check if there is a valid plan, and if not, call the plan_init to create one.

Alternatively, if this is too complex, it would have saved us some time if the attributes were read-only properties.

thanks for making this available! greetings from PTB Berlin (MR reconstruction)

@ahbarnett
Copy link
Collaborator

Hello Felix,
Yes, by attempting things not described in the use examples, you'd discovered new ways of breaking things that we had not thought of :) (common with software). I agree that detecting changes and auto-replanning would be too complicated (eg you wouldn't expect that of an FFT lib; and FFTW's storage of multiple plans leads to many complications). Changing ntrans needs a new FFT plan (unless DUCC is used... so if we switch entirely to DUCC this could be easier). I like your read-only solution; I did not write our ctypes interface so will ping @janden about this when he gets time.

Meanwhile you could always do the loop over the batches yourself (having planned ntrans=1). I wonder how much slower that would be?

@janden
Copy link
Collaborator

janden commented Oct 17, 2024

Agree that auto-replanning is a bit too much here.

@fzimmermann89 I'm not sure what you mean by read-only here? We can make the attributes semi-private (i.e., n_trans_n_trans) and expose n_trans as a property, but this wouldn't prevent a user from modifying _n_trans. That being said, it would make it slightly more obvious that these things shouldn't be touched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants