search

NIFTI-Imaging / nifti_clib

C libraries for NIFTI support
Other
33 stars 24 forks source link

ENH: add nifti_image_write_status #147

Closed afni-rickr closed 2 years ago

afni-rickr commented 2 years ago

add NIFTI-1 and -2 versions of nifti_image_write_status

gdevenyi commented 2 years ago

Just a thought, have you considered converting nifti_image_write to a thin wrapper which calls nifti_image_write_status and discards the return bits? This could reduce code duplication and ensure a unified path for code testing etc.

afni-rickr commented 2 years ago

@gdevenyi Yes, there are many such bits to possibly do. Currently, nifti_image_write does call _status, except that it goes through 2 intermediate layers first. We could cut out those layers, though it is weighed against modifying functions that work as they are. But if you think _write should call _status, I will make that change. I will duplicate most of this in the NIFTI-1 library, too. Thanks.

gdevenyi commented 2 years ago

I think for consistency/uniformity it makes sense for nifti_image_write and nifti_image_write_status to take the same pathways through the code wherever possible.

My only concern is that based on your description, changing nifti_image_write to wrap nifti_image_write_status is a big-ish change to the existing codeflow, as they're implemented differently? I can see either side of this discussion winning out, perhaps it depends on what the next version # of the release would be :)

afni-rickr commented 2 years ago

The addition of _engine is already modifying the code for most of the image_write functions, including nifti_image_write, so this is already a somewhat impactful change. Since nifti_image_write is already calling the engine (indirectly), it would make sense to just be direct about it. I am thinking to make a similar nifti_image_write_bricks_status function, too, esp. since write_bricks is already relying on _engine. So the _engine function is the real change (for NIFTI-1 and -2), but it will lead to lots of little changes, including in the examples.

afni-rickr commented 2 years ago

For the most part, serious errors have always been printed to stderr unconditionally. There are some cases of if( g_opts.debug > 0 ), but not so many. Also, the _engine, as with the old hdr_img2 code will generally print the reason for failure. Those could be wrapped in a test for debug > 0 (probably not 1), but that would be a change.

hjmjohnson commented 2 years ago

@afni-rickr I am OK with not changing historical behavior in the PR.

I recommend that a future PR consider not writing to terminal stderr from library functions. Where should those messages be displayed for a GUI application?

afni-rickr commented 2 years ago

@hjmjohnson sure, a future PR preventing default stderr writing from the library seems reasonable. For a GUI, are those messages simply not seen? If that is what happens, such a PR would not affect them. We can ponder it.

afni-rickr commented 2 years ago

@hjmjohnson I am going to make a quick parallel update for nifti_image_write_bricks (which is already calling _engine, eventually). Would you like a test added for such a failure case? I have done local testing using a read-only output directory.

hjmjohnson commented 2 years ago

@afni-rickr I think that test may be hard to write. I'll be satisfied with historical testing that did not test the error status.

Thank you for you assistance with making these changes.

afni-rickr commented 2 years ago

@hjmjohnson Okay, I think this is ready. Would you like a last peek at the recent commits? Please feel free to merge if you are happy (or at least moderately satisfied :).