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

BSMEM prior image #10

Open
FerreolS opened this issue Oct 4, 2021 · 22 comments
Open

BSMEM prior image #10

FerreolS opened this issue Oct 4, 2021 · 22 comments
Labels
bug Something isn't working

Comments

@FerreolS
Copy link
Member

FerreolS commented Oct 4, 2021

BSMEM prior image is missing (see bug trac #811 )
By default it uses the init image as prior but we should provide at least a Gaussian prior parametrized by the FWHM.
Whether it can be easily done in command line (using the flag -mw), I'm not sur it is possible through the OImage format.

@FerreolS FerreolS self-assigned this Oct 4, 2021
@FerreolS
Copy link
Member Author

FerreolS commented Oct 4, 2021

The prior image should be stored in an hdu identified in RGL_PRIO keyword

@buthanoid
Copy link
Member

buthanoid commented Oct 4, 2021

There exists an RGL_PRIO select list in SoftwareSettingsPanel, but disabled. Also it is empty.
Should it list the same thing as the jComboBoxImage which is the select list for Input Images ? Note that this jComboBoxImage uses the field selectedInputImageHDU in IRModel to store and change the input image.

Could be re-enabled by setting it as "supported standard keywords" in BsmemInputParam.

The Combo box could use the same IRModel.fitsImageHDUs field as jComboBoxImage for its items.
Then it would need a similar function to IRModel.setSelectedInputImageHDU to modify the OiFitsFile (hdus and keywords). Note that in this function there is a code commented as "Prune OIFits images to keep ONLY ONE image". Maybe this would need update to NOT removing the RGL PRIO image.

It seems that the Hdus in a OIFitFile are just a list so we could easily add a RGL PRIO hdu.

@bourgesl
Copy link
Member

bourgesl commented Oct 4, 2021

Actually RGL_PRIO is defined as a standard keyword, but explicitely disabled by BsmemInputParam.supportsStandardKeyword().

However this String keyword is special as it should be handled in the GUI as an Image Selector (+/- load image buttons) and display a list of candidate images (same list as INIT_IMG), so this keyword should be handled (hard-coded) in SoftwareSettingsPanel in particular, but this behaviour could be replicated for specific software keywords (like WISARD to give a support image).
I think the result images should not interfere in RGL_PRIO and other software-specific keywords. should INPUT images be present ?
Finally some refactoring is needed to clarify HDU / hduName (renaming) in IRModel and check if all combo boxes are well refreshed (consistent naming)

@buthanoid
Copy link
Member

branch https://github.com/buthanoid/oimaging/tree/bsmem-rgl-prio

I had to modify the function IRModel.setSelectedInputImageHDU to prevent it from deleting the RGL PRIO hdu. By the way this function was doing weird things, for example checking that the item selected is contained in the list of loaded images inputs, but this should always be, since the list of items is constructed from the loaded image inputs, and anyway we have a HDU variable so we can use it no matter it exists or not in some list. This is why I removed the checking part.

@buthanoid
Copy link
Member

buthanoid commented Oct 5, 2021

For the rest I did not mimic exactly the input image way, for example I used a FitsImageHDU value to represent the "[No Image]" item choice, instead of mixing String with FitsImageHDU in the same list.
I chose not to use this Null HDU in IRModel and to convert to a simple "null" value", so it work for both following scenarios :

  • we are not using bsmem, we are not using rgl prio
  • we are using bsmem, we chose the No Image item

in both scenarios IRModel has to not include an rgl prio hdu in the oifitsfile.

@buthanoid
Copy link
Member

Still some work to do :

  • test that the file contains corrects Hdus in various scenarios
  • mimic the input image behaviour, that when we chose an image it gets displayed on view panel

@bourgesl
Copy link
Member

bourgesl commented Oct 6, 2021

branch https://github.com/buthanoid/oimaging/tree/bsmem-rgl-prio

I had to modify the function IRModel.setSelectedInputImageHDU to prevent it from deleting the RGL PRIO hdu. By the way this function was doing weird things, for example checking that the item selected is contained in the list of loaded images inputs, but this should always be, since the list of items is constructed from the loaded image inputs, and anyway we have a HDU variable so we can use it no matter it exists or not in some list. This is why I removed the checking part.

@gmella Any idea why setSelectedInputImageHDU() needed to check if the given hdu was present in the current list of imageHDUs (checksum...), = consistency check ?

@bourgesl
Copy link
Member

bourgesl commented Oct 6, 2021

For the rest I did not mimic exactly the input image way, for example I used a FitsImageHDU value to represent the "[No Image]" item choice, instead of mixing String with FitsImageHDU in the same list. I chose not to use this Null HDU in IRModel and to convert to a simple "null" value"...

I would prefer having the same approach to handle Null images in all cases (INIT_IMG, RGL_PRIO).
Using your approach NULL_IMAGE_HDU seems better, could you do the same for the input image ?

Finally I had a look at your fork branch : code looks good, not tested.
However doing a PR is the good approach to review / test changes... instead of giving links to your fork branches.
Please Format the code on save.

@gmella
Copy link
Member

gmella commented Oct 6, 2021

@gmella Any idea why setSelectedInputImageHDU() needed to check if the given hdu was present in the current list of imageHDUs (checksum...), = consistency check ?

This was probably done to avoid stacking of duplicated images. The basic associated use case is to run a process with the same input params. A result also can be the same for small input changes and this would not add the output image because the GUI already has it.

@bourgesl
Copy link
Member

bourgesl commented Oct 7, 2021

TODO: test case INIT_IMG = RGL_PRIO (avoid duplicates)

@bourgesl
Copy link
Member

bourgesl commented Oct 8, 2021

Sorry I integrated too quickly the PR, let's fix the bug later:

handle case INIT_IMG = RGL_PRIO to avoid duplicated HDU (same name, 2 HDUs)

@bourgesl bourgesl assigned buthanoid and unassigned FerreolS Oct 8, 2021
@bourgesl bourgesl added the bug Something isn't working label Oct 8, 2021
@bourgesl
Copy link
Member

bourgesl commented Oct 8, 2021

Beta release: http://jmmc.fr/~betaswmgr/OImaging/

@FerreolS FerreolS reopened this Oct 11, 2021
@buthanoid
Copy link
Member

buthanoid commented Oct 11, 2021

I think in fr.jmmc.oitools.model.OIFitsFile we should manage the INIT_IMG and RGL_PRIO HDUs. With setters we could check when both names are the same (then we need only one HDU), when one HDU is not pointed anymore and can be removed, etc..
Small problem is that the class extends FitsImageFile, where you find a method which gives you full control access to the list of HDUs, and it is used.
Also FitsImageFile sounds like more general than OIFitsFile, like "above OI". But I think HDU_NAME is an OI concept, so if FitsImageFile is always used with OI concepts (it uses FitsImageHDU which have HDU NAME), maybe we could incorporate a uniqueness check here, and OIFItsFile would inherit.

OR, we just give systematic different HDU names like a one letter prefix I for InputImg and R for RglPrio.

@bourgesl
Copy link
Member

I do not see why oitools should manage such OImaging logic that is caused by IRModel.selectHDUs():

private void selectHDUs() {
        oifitsFile.getFitsImageHDUs().clear();
        if (selectedInputImageHDU != null) {
            oifitsFile.getFitsImageHDUs().add(selectedInputImageHDU);
        }
        if (selectedRglPrioImage != null && selectedRglPrioImage != NULL_IMAGE_HDU) {
            oifitsFile.getFitsImageHDUs().add(selectedRglPrioImage);
        }
    }

If selectedInputImageHDU == selectedRglPrioImage: do not add selectedRglPrioImage ? or check HDU_NAME values too ?

@buthanoid
Copy link
Member

buthanoid commented Oct 12, 2021

I did it, it works fine for the input file (only the primary HDU contains an image when both init and rgl are same).
but BSMEM gives back a wrong FITS file with 3 images :

  • primary : the output image (this is correct)
  • xtension image and hdu name "toto" : this is correct
  • xtension image and hdu name "toto": this is incorrect because there already exists one, and there is no EXT_VER keyword to distinguish the two.

Of course this is a BSMEM problem since it happens also when the input files have duplicate hdus.

buthanoid@0de04c1

@bourgesl
Copy link
Member

bourgesl commented Oct 13, 2021

I propose to adopt another approach:

  • put 2 HDUs in the input OIFITS file (as before)
  • but ensure HDU_NAME are unique within the file ie fix the HDU_NAME of the RGL_PRIOR image to add 'PRIOR_...' in case of conflict

@buthanoid
Copy link
Member

buthanoid commented Oct 13, 2021

Problem is the two hdus are the same object. If I rename one HDU I rename both.

@buthanoid
Copy link
Member

I think it is best if OImaging try to respect the spec. If BSMEM creates duplicates HDU, as long as OImaging is able to read them correctly, then it is a BSMEM problem

@bourgesl
Copy link
Member

I am OK to fix OImaging GUI (no duplicated HDU) now and fix BSMEM later (in our JMMC-OpenDev repo)
We should look how to clone FitsImageHDU/FitsImage (already supported or not ?)

@buthanoid buthanoid removed their assignment Nov 2, 2021
@bourgesl
Copy link
Member

bourgesl commented Nov 3, 2021

With the new FitsImageHDU copy constructor, it is time to fix this issue:

  • always add 2 HDUs
  • if the RGL_PRIOR == INIT_IMG, copy the HDU and rename it accordingly: HDU_NAME='PRIOR_...'

@buthanoid
Copy link
Member

#38

@bourgesl bourgesl linked a pull request Nov 8, 2021 that will close this issue
@jsy1001
Copy link

jsy1001 commented Feb 14, 2022

I have tested BSMEM with OImaging, for both RGL_PRIO = INIT_IMG and RGL_PRIO != INIT_IMG. In both cases BSMEM writes two image HDUs to the output FITS (in positions 2 and 3), for INIT_IMG and RGL_PRIO respectively. This mirrors how OImaging writes the input file, where there are always separate image HDUs for INIT_IMG (the primary HDU) and RGL_PRIO (the penultimate HDU).

What is the desired behaviour for BSMEM? Do you want only one image in the output file when RGL_PRIO = INIT_IMG? This should probably be clarified in the interface document.

BTW, it would be helpful if OImaging would write the name of the image to the EXTNAME keyword as well as HDUNAME (except for the primary HDU where EXTNAME is not allowed by the FITS standard).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants