Skip to content

Conversation

@rijobro
Copy link
Contributor

@rijobro rijobro commented Jun 12, 2020

@evgueni-ovtchinnikov how about this. Change _storage_scheme to _default_storage_scheme. If using the static method, return the default. However, if the object exists, return the corresponding type.

I've added the test, which fails with the old code, but passes with these changes:

    AcquisitionData.set_storage_scheme("memory")
    ad = AcquisitionData(raw_data_file)
    AcquisitionData.set_storage_scheme("file")
    test.check_if_equal("memory", ad.get_storage_scheme())

@rijobro rijobro force-pushed the fix_get_storage_scheme branch from f92c833 to 9f2094a Compare June 12, 2020 09:05
@KrisThielemans
Copy link
Member

strategy seems ok to me, but is it necessary to change the C++ name (you don't on the python side)? One more thing that breaks backwards compatibility, for not so good reasons?

@rijobro
Copy link
Contributor Author

rijobro commented Jun 12, 2020

I can leave the naming as it is if you'd like, but it seems confusing to me.

Keeping the old name makes it unclear as to whether you're returning the default scheme or the scheme for the current object.

I didn't change the naming on the python side because it handles both cases - if object is None, return default scheme, else return scheme for specific object.

Thinking about it, I should probably update the matlab side...

@KrisThielemans
Copy link
Member

Keeping the old name makes it unclear as to whether you're returning the default scheme or the scheme for the current object.

I didn't change the naming on the python side because it handles both cases - if object is None, return default scheme, else return scheme for specific object.

I don't see the difference between C++ and Python in that respect. PETAcquisitionData::storage_scheme() has no object, so will return the default. acq_data.storage_scheme() will return the value for the object. That's what you do in Python as well:

AcquisitionData.set_storage_scheme(scheme)

I agree that the default_storage_scheme() name might be better, but it does need some more changes in demos and UserGuide (ok, the latter will have to be updated in any case!).

In any case, I want to have the same names in Python/MATLAB/C++.

Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

needs changes in UsersGuide and CHANGES. see also comment

Comment on lines +111 to +112
temp = PET.AcquisitionData();
temp.set_storage_scheme('memory');
Copy link
Member

Choose a reason for hiding this comment

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

I'd be amazed if this changes the default. In C++,Python you need PET.AcquisitionData.set_storage_scheme

@KrisThielemans
Copy link
Member

@evgueni-ovtchinnikov is this PR still valid (I'm sure it'd need changes). If so, please assign to 3.1

@evgueni-ovtchinnikov
Copy link
Contributor

I am not convinced we need this (happily lived without it for 4+ years by now), so closing - but feel free to reopen if disagree, just try to make a stronger case for it.

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

Successfully merging this pull request may close these issues.

Changing storage scheme of AcquisitionData

3 participants