Package broken by RegularGrid -> RectGrid rename in ODL

[adler-j]

The pickled geometries need to be updated to reflect this change.

[davlars]

How are the geometries defined as RegularGrids? I can't seem to reproduce any errors, and I should working on an up-to-date odl master version. Or would this cause problems further down?

[adler-j]

Well RegularGrid has been renamed to RectGrid, and it is used inside the geometries (technically inside the angle partition etc). This has been done on master a few days ago, odl 0.5.3 still works.

[ozanoktem]

This needs to be resolved asap, using ODL 0.5.3 is just a temporary procedure. I want to have a plan for for how to resolve this issue, i.e. to ensure the pickled geometries properly reflect this API change.

[davlars]

Sorry to ask again, but just to understand the conflict in the pickled geometries. When originally created, they are now defined as:

 angle_partition = odl.uniform_partition(startAngel, endAngel, nProj)

detector_partition = odl.uniform_partition(startCorner, endCorner, nPixels)

geometry = odl.tomo.HelicalConeFlatGeometry(angle_partition,
                                            detector_partition,
                                            src_radius=sourceAxisDistance,
                                            det_radius=detectorAxisDistance,
                                            pitch=pitch_mm,
                                            pitch_offset=-2)

(see HelicalSkullCT_GPUMCI.py for reference)

When I then load the pickled geometries I just see them as uniform partitions - do you mean that RegularGrid vs. RectGrid inside these? Or do I need to change the definitions?

Anyway using an up-to-date odl-master-repo (which still is ODL 0.5.3 if I understand it correctly?) I've re-created pickled geometries, which have been saved in the correct repo with identical name as before (i.e. running adutils.load_data_from_nas should give you the new data, or just go /path/to/nas/data/Simulated/120kV and fetch *geometry*). If everything is "under the hood" of odl.uniform_partition this should work (or?)

Has this solved the issue or am I misunderstanding the definitions?

[kohr-h]

The problem is under the hood, namely that the pickled objects store an instance of a class that doesn't exist any more. Hence, unpickling it will fail. It's a bit bad to have these API changes blowing up stuff like this, but that's life (currently). I think one thing you could do for yourself is adding ODL as a git submodule that points to a commit where you know thinks work as they currently are. For example, in this case, point to a commit before the API change, and verify things still work before advancing that pointer.

The current ODL master is version 0.5.4.dev0, so depending on 0.5.3 should be fine with the old pickled geometries.

Anyway, I think the larger issue is that pickling is not a very good way to store meta-information because it is implementation-dependent as made obvious by this issue. Jan-Willem is working on a TOML-based specification for geometries. That would help in solving this kind of issues.

[davlars]

Right. So if pickling, I/we should pickle them in the same version that we're then (afterwards) intending to run reconstructions in? Short term (or as long as we're now dealing with pickled geometries) new geometries have been pickled and uploaded. I don't seem to have any apparent issues as of now with these: has anyone else tried to load these?

Creating the geometries is very fast computationally so we can in principle code it into the utilities of this package, but it is really good to be able to save geometries when you simulate (to not confuse yourself when your trying old data that's lying around).

[kohr-h]

Sure, don't get me wrong. Although pickling is not a very good meta-info storage, it's still far better than not storing at all. It's just kind of painful when API changes are done. So as a general remark, and perhaps you already do that, it's good to also store the exact script that generated the data.

[davlars]

Right. I agree: keeping track of whatever code was used to generate the data should also be part of our "good code practice".

But getting back to the RegularGrid vs RectGrid class change, is this overcome by updating the pickling (i.e. replacing the "old" geometry-files with new ones)? If it works, it's an ugly solution, but it would anyhow enable us to keep working with the data...

[kohr-h]

To be precise, it's a TensorGrid -> RectGrid change and the removal of RegularGrid, so you would really have to re-run the code that generates the pickled files. The uniform_* functions still work the same way, so you probably don't need to change the scripts.

[adler-j]

As @kohr-h mentioned, the easiest way to fix this is to simply run the script that generated them (without creating the actual MC data).

In the long run, we should likely implement the __reduce__ method which solves exactly this issue.

[davlars]

Thanks for the input. So short term: the geometries (should) have been updated. I'm leaving the issue open until we fix it in a more general sense, by e.g. what you suggested @adler-j

[kohr-h]

In the long run, we should likely implement the reduce method which solves exactly this issue.

Partially, that is. In this case, it wouldn't actually solve the issue because there would still be a pickled RegularGrid as part of the pickled geometry (it's an init argument). A pickle file is nothing but a text file that is interpreted to re-create the pickled object, and it explicitly mentions class and method names. So any API change that touches something contained in a pickle file breaks it. If the change is a rename, one can do something like sed -i "s/OldName/NewName/" pickle_file.p on all files to replace the name. But in our case it's more complicated since the change went beyond simple renaming.

[adler-j]

Well we technically the argument is a RegularPartition, so the arguments have not changed (given that the partition was constructed using uniform_partition). So this issue would indeed be solved.

Further, it seems that there is some kind of versioning built in that we could use. A bit hard for me to look into given that China blocks both Google and stack overflow :S

[aringh]

@davlars have you updated the code or just the data? I see no commits here on git, and when I tried to copy the data again from the nas I still get

AttributeError: Can't get attribute 'RegularGrid' on <module 'odl.discr.grid' [...]

[davlars]

I've updated the data, not the code. You're running with newly downloaded data? I'll check whether there's something from the old stuff lying around...

[aringh]

Yes, I used the instructions to get the data from the nas just 10-15 minutes ago :-)

[davlars]

Ok. I'm checking the versions of odl and the data right now. I'll try to post an update asap.

[kohr-h]

Well we technically the argument is a RegularPartition, so the arguments have not changed (given that the partition was constructed using uniform_partition). So this issue would indeed be solved.

Right, if you implement __reduce__ all the way down, it will actually solve the issue.

Further, it seems that there is some kind of versioning built in that we could use. A bit hard for me to look into given that China blocks both Google and stack overflow :S

So? No coding possible without Google and SO? :-P For search, use DuckDuckGo, if that's not blocked.

[davlars]

I've just re-created the geometries we want with odl 0.5.4.dev() (the're saved under the same name in the same folder) and was able to unpickle them again. Should work if you do the load_data-call again (or just transfer the *.p-files in the /path/to/nas/data/Simulated/120kV/-folder).

Is it working for you now @aringh?

[aringh]

It works :-) Thank you!

[adler-j]

Great work @davlars!