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

Improve Image identification in FitsImagePanel views #8

Closed
bourgesl opened this issue Sep 29, 2021 · 11 comments · Fixed by #34
Closed

Improve Image identification in FitsImagePanel views #8

bourgesl opened this issue Sep 29, 2021 · 11 comments · Fixed by #34
Assignees
Labels
enhancement New feature or request
Projects

Comments

@bourgesl
Copy link
Member

The FitsImagePanel displays a too complicated title (file name ...) from FitsImage.getFitsImageIdentifier()
It would be better to use the HDU_NAME (or renamed by the GUI) for the image, but also indicate the associated result.

Two solutions:

  • modify FitsImagePanel to have a new ID field given with the image and displayed in place or as a prefix in the chart's title
  • use setFitsImageIdentifier() to define the user-defined or better identifier in the OImaging code

To be investigated as the ViewerPanel only deals with FitsImageHDU so no mean to get its ServiceResult.

@bourgesl bourgesl added this to To do in Oimaging Sep 29, 2021
@bourgesl bourgesl moved this from To do to Specs / Design in Oimaging Sep 29, 2021
@buthanoid
Copy link
Member

buthanoid commented Sep 30, 2021

In IRModel.postProcessOIFitsFile ?
We could loop on FitsImages and call setFitsImageIdentifier. We would have both information about the service and the oifitsfile (we could totally add the service result as a parameter of postProcessOIFitsFile)
For the label, what about composing some interesting keywords like the algorithm used, the target, the hdu (primary or initial image), and the current count of images indexes ? And not printing the file name as currently, which is too long. example :
Id: PI_GRU-WISARD-PRIMARY-img(1/1)#3
The last #3 could be the index in the result table, for example.
After that we can still reflect about letting the user enter a completely personalized label.

@bourgesl
Copy link
Member Author

What should be displayed for input images ? ie images loaded manually not coming from results.
If keywords change, like result ID, the image identifier should be updated as well...

@bourgesl bourgesl moved this from Specs / Design to To do in Oimaging Oct 1, 2021
@bourgesl bourgesl moved this from To do to Specs / Design in Oimaging Oct 1, 2021
@buthanoid
Copy link
Member

Note : There is also different labels for images in the "Image Input list".
There is also image labels in the view panel, where you can choose which image you want to display (the input one (if any) or the result one).

@FerreolS
Copy link
Member

Two case:

  • OUTPUT image -> INDEX_HDUNAME
  • FITS image -> FILENAME_HDUNAME

If no hduname use its number

@buthanoid
Copy link
Member

buthanoid commented Oct 18, 2021

  • we need to check the behaviour of the images that are not added to the list because there already exist a check-sum-equal one, or the images that are renamed because the HDU name already exist in the list (the list I am talking about is IRModel.getFitsImageHDUs())
  • HDU_NAME suggests updating the name when HDU_NAME updates, similar thing for INDEX

@buthanoid
Copy link
Member

buthanoid commented Oct 19, 2021

Problem:
when you click on "Run", it calls the function OIFitsWriter.writeOIFits, which himself call OIFitsWriter.createHDUnits. This last one creates HDUs from the ones found in IRModel.oifitsfile. Problem is that it calls FitsImageWriter.createImageData which always rename the image. Why ? At least in OImaging it does not serve because the file resulting from the algorithm will be loaded and its filename will be used for the image identifiers, so the filename of the input file will not appear in the result.
Also this renaming, while not appearing in the result of the run, actually change the name of the initial image used. This image can be the result of a previous run, which thus get renamed, which is confusing.
i propose to remove this renaming in FitsImageWriter, unless it is used for something

@buthanoid
Copy link
Member

This reflect the weakness of just calling setFitsImageIdentifier once to modify the display. But I think it is an actual issue, not only impacting the development of the ticket here

@bourgesl
Copy link
Member Author

As you described, the FitsImage identifier was aim to indicate 'filename ... 0/1..." so it is automatically updated by FitsImageLoader and FitsImageWriter to compute its value once.

I am a bit lost in your problem description as several Object instances are used in the workflow:

  • input OIFits (in memory) - 1
  • loaded Fits images (in memory) - 2
  • saved OIFits (in /tmp), only used by UWS - 3
  • loaded OIFits result (in /tmp) - 4

I think you should modify the FitsImage identifier once an image is loaded (in memory), so you just overwrite the value by what you want.
It should be done in IRModel loadImage(), loadOIFits (input) and addServiceResult().

I agree with you that 3 will modify the FitsImage identifier of images loaded (2), so it seems necessary to add a new runtime flag in oitools to discard automatic update in FitsImageWriter...

See https://github.com/JMMC-OpenDev/oitools/blob/master/src/main/java/fr/jmmc/oitools/model/DataModel.java that has few OITools flags.
You should add a new constant flag in FitsImageWriter to guard the following lines:

  236:  image.setFitsImageIdentifier(fileName + '#' + hduIndex);
  251:  image.setFitsImageIdentifier(fileName + '#' + hduIndex);

@buthanoid
Copy link
Member

Yes what you described is what I meant, -3 modify -2.
I agree with the constant flag, it seems the safest since the writeOIFits functions is called in different situations : TestMergeUtils.merge, WriteOIFitsTest.writeLoadCompare, ViewerPanel.exportOIFits..
And the fitsImageIdentifier is also used in different situations.
We only know that in this specific situation it will be harmless to not update the identifier with the name of the temporary file

@buthanoid buthanoid moved this from Specs / Design to In progress in Oimaging Oct 20, 2021
@buthanoid
Copy link
Member

@buthanoid
Copy link
Member

Not finished. I only renamed the images from the result for now.

@buthanoid buthanoid added the enhancement New feature or request label Nov 2, 2021
@buthanoid buthanoid linked a pull request Nov 2, 2021 that will close this issue
@bourgesl bourgesl moved this from In progress to Done in Oimaging Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

3 participants