Fits header after stacking

[btschumy]

Kevin,

Here is my new code. Hopefully I've done the pull request ok. Since I didn't initially make my changes in a fork, I had to fork, copy my code over, push and then generate the pull request. Loos like I have everything here.

[btschumy]

I can make those changes.

I was thinking of moving the reflection code into a separate method in ZProjector. Seems a little strange to me to move it into its own class given there would only be that one method in it. However I agree it would make it harder to lose. Is that our preference?

Regarding the handling of the exceptions, what is the policy here. Should these errors just get logged, or do you want to display an alert notifying the user?

Bill

On Oct 7, 2022, at 4:29 PM, keastrid @.***> wrote:

@keastrid requested changes on this pull request.

Looks good! Just a few minor nitpicks...

In ij/src/main/java/ij/plugin/ZProjector.java https://github.com/AstroImageJ/astroimagej/pull/56#discussion_r990532234:

@@ -359,9 +358,25 @@ public void doProjection() { projImage = makeOutputImage(imp, fp, ptype); }

• merger.setHeader(projImage);
• if(projImage==null)
• // Merge the headers
• // BT: Might move this into a method for cleanliness Yes, moving this out to a separate method would be good. It will be harder to lose if added as its own class in ij/astro/util.

In ij/src/main/java/ij/plugin/ZProjector.java https://github.com/AstroImageJ/astroimagej/pull/56#discussion_r990532751:

• } catch (ClassNotFoundException e) {
• System.out.println(e);
• } catch (NoSuchMethodException e) {
• System.out.println(e);
• } catch (InvocationTargetException e) {
• System.out.println(e);
• } catch (IllegalAccessException e) {
• System.out.println(e);
• }; Since these are all the same, you can go ahead and collapse them.

It may be useful in addition to the stack trace to provide the user a message with IJ#error, to let them know that header changes were not applied.

— Reply to this email directly, view it on GitHub https://github.com/AstroImageJ/astroimagej/pull/56#pullrequestreview-1135118586, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADNQNBKAQL5HWIWXUUG3K3WCCP5HANCNFSM6AAAAAAQ7WBIX4. You are receiving this because you authored the thread.

-- Bill Tschumy Otherwise -- Longmont, CO www.otherwise.com

[keastrid]

It's not that large, so as long as it is properly decorated with @AstroImageJ a separate method in ZProjector is fine.

Error logging is preferred (makes it easier to debug), and you have that. An alert should be shown to the user if the stack has headers and they failed to merge.

[keastrid]

Thanks for making the changes! Here is a patch to fix the BITPIX added to the header, which can be applied like so:

With the patch, header merging seems good!

That said, having looked into ZProjector to see why it was reporting the merged image as 16bit, I don't think in its current state you're going to get a proper image out of this, for 8 and 16bit images it's truncating the floating point values to their integer counterpart. The call to ZProjector#makeOutputImage in doProjection will need some more conditions put in place, its really only safe to call when the method is max, min, or median. Averaging and standard deviation should give floats, and summing could require a bump in integer type. Currently only st. dev. and summing are properly handled.

@karenacollins How do you want to handle this? Header merging is good once the patch is applied. The above issue seems to only affect images that have not converted to floats, so a workaround is for users to set the image type to -32 before running the merger.

[karenacollins]

Hi Kevin,

Can we add code to force a 32-bit output for all input cases?

Karen

On 10/10/2022 2:49 AM, keastrid wrote:

  Thanks for making the changes! 
      Here is a patch to fix the BITPIX added to the header,
    which can be applied like so:

  With the patch, header merging seems good!
  That said, having looked into ZProjector to see why
    it was reporting the merged image as 16bit, I don't think in its
    current state you're going to get a proper image out of this,
    for 8 and 16bit images it's truncating the floating point values
    to their integer counterpart. The call to ZProjector#makeOutputImage in
    doProjection will need some
    more conditions put in place, its really only safe to call when
    the method is max, min, or median. Averaging and standard
    deviation should give floats, and summing could require a bump
    in integer type. Currently only st. dev. and summing are
    properly handled.
  @karenacollins
    How do you want to handle this? Header merging is good once the
    patch is applied. The above issue seems to only affect images
    that have not converted to floats, so a workaround is for users
    to set the image type to -32 before running the merger.
  —
    Reply to this email directly, 
      view it on GitHub, or 
      unsubscribe.
    You are receiving this because you were mentioned.Message
      ID: ***@***.***>
  [

{ @.": "http://schema.org", @.": "EmailMessage", "potentialAction": { @.": "ViewAction", "target": "https://github.com/AstroImageJ/astroimagej/pull/56#issuecomment-1272851945", "url": "https://github.com/AstroImageJ/astroimagej/pull/56#issuecomment-1272851945", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { @.": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

[keastrid]

Pushed a fix for ZPRojector, once the patch is applied here I'll go ahead and merge.

[btschumy]

Kevin,

Are we good with my changes? To be honest, I haven't submitted changes to an open source project via pull requests before. Are you supposed to close this request after integrating my changes, or are you still waiting on me doing something?

[keastrid]

Merged, also applied the fix BITPIX in the header.