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

DM-14732 Regions appear on subsequent afw Displays with Firefly backend #12

Merged
merged 2 commits into from Jun 8, 2018

Conversation

stargaser
Copy link
Contributor

@stargaser stargaser commented Jun 8, 2018

  • Add a _clearImage method to the Firefly backend
  • When using show_fits, clear the list of predefined overlays that are carried over to new image displays

The _clearImage method did not fix the issue of having regions overlays appear on new displays, but it is retained as a potentially useful method in the backend.

@stargaser stargaser requested a review from TallJimbo June 8, 2018 15:32
Copy link
Member

@TallJimbo TallJimbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I left a few minor comments.

@@ -140,7 +147,8 @@ def _mtv(self, image, mask=None, wcs=None, title=""):
self._fireflyFitsID = _fireflyClient.upload_data(fd, 'FITS')

ret = _fireflyClient.show_fits(self._fireflyFitsID, plot_id=str(self.display.frame),
Title=title, MultiImageIdx=0)
Title=title, MultiImageIdx=0,
PredefinedOverlayIds=' ')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is really supposed to be a single space, rather than an empty string or some other more typical custom value, it probably merits a code comment explaining why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does have to be a space, or else the Javascript side discards the parameter...I'll add a comment

"""
self._client.dispatch_remote_action(channel=self._client.channel,
action_type='ImagePlotCntlr.deletePlotView',
payload=dict(plotId=str(self.display.frame)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that you trying calling this in _mtv and it didn't help with this particular problem, but if it does no harm it may be exactly what's needed to fix DM-14695, in that it seems to be a programmatic way to do exactly what I was doing in the GUI in my workaround (though I gather there may be different fix for that issue already in the works).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does fix the mask but it deletes the image entirely. @robyww and I looked at this and decided this was undesirable. Once you have, say, three displays up, and you mtv again to the first one, displays 2 and 3 shift to the left and display 1 appears on the right. Trey has a server-side fix that we will do instead.

@@ -125,6 +125,13 @@ def __init__(self, display, verbose=False, url=None,
def _getRegionLayerId(self):
return "lsstRegions%s" % self.display.frame if self.display else "None"

def _clear_image(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should probably be _clearImage, since this class inherits from a base class that definitely uses the camelCase conventions used in Science Pipelines code, but it's a bit of a grey area and I won't insist (especially since it's a private method).

* add comment explaining space in parameter string

* change method to _clearImage
@stargaser stargaser merged commit cc3a82e into master Jun 8, 2018
@ktlim ktlim deleted the tickets/DM-14732 branch August 25, 2018 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants