Icchen/79 render vector overlay

[I-Chenn]

This PR is to resolve #79.

@kswang1029 @confluence, I haven't finished this issue yet, but would like to ask a question before going on.

I have added configure_vector_overlay, set_vector_overlay_color, set_vector_overlay_colormap, apply_vector_overlay and clear_vector_overlay to image.py.

The configure_vector_overlay is a huge method with 9 parameters within since VectorOverlayStore in the frontend stuck all the parameters into setVectorOverlayConfiguration action to set the Configuration panel. Nevertheless, for the Styling panel, it seems that all the parameters are divided into respective actions.

My question is: do you prefer the way of dividing these actions into different Python functions? or do you prefer to stuck all these actions into one function , like a set_vector_overlay_styling function, for setting up the styling panel?

[confluence]

I think that this split is OK for now. You can, however, eliminate the boolean parameters enabling or disabling certain features, because you can deduce them from the other parameters. Set pixel_averaging_enabled to True if pixel_averaging is not None, otherwise False, and so on (in this particular case, make sure you change the validation for pixel_averaging so that None is allowed). You can do the same for threshold.

For debiasing, are both error values required? If the user specifies one, does it make sense to use it for both errors, or are they likely to be completely different magnitudes? Does either of them have a sensible default? Depending on the answer to this, you can enable debiasing either if both error parameters are given, or if at least one is given.

Everything under Styling (except the colour, which I think it makes sense to keep separate for consistency) should maybe be grouped under a set_vector_overlay_line, or maybe set_vector_overlay_style (in which case set_contour_dash should perhaps be renamed to set_contour_style to match).

[kswang1029]

I think that this split is OK for now. You can, however, eliminate the boolean parameters enabling or disabling certain features, because you can deduce them from the other parameters. Set pixel_averaging_enabled to True if pixel_averaging is not None, otherwise False, and so on (in this particular case, make sure you change the validation for pixel_averaging so that None is allowed). You can do the same for threshold.

For debiasing, are both error values required? If the user specifies one, does it make sense to use it for both errors, or are they likely to be completely different magnitudes? Does either of them have a sensible default? Depending on the answer to this, you can enable debiasing either if both error parameters are given, or if at least one is given.

errors for stokes Q and U could be different so we need them as separate parameters.

Everything under Styling (except the colour, which I think it makes sense to keep separate for consistency) should maybe be grouped under a set_vector_overlay_line, or maybe set_vector_overlay_style (in which case set_contour_dash should perhaps be renamed to set_contour_style to match).

Vector overlay can be line segments or filled squares. Need to consider this in the naming convention.

[confluence]

errors for stokes Q and U could be different so we need them as separate parameters.

Yes, there will definitely be two parameters. But is the user ever likely to use the same value for both, or is it very unlikely?

(Actually, it probably doesn't matter -- it would be way too magical to make one default to the other, so we shouldn't do it either way. :) )

[kswang1029]

errors for stokes Q and U could be different so we need them as separate parameters.

Yes, there will definitely be two parameters. But is the user ever likely to use the same value for both, or is it very unlikely?

(Actually, it probably doesn't matter -- it would be way too magical to make one default to the other, so we shouldn't do it either way. :) )

If stokes Q and U images are created with different clean processes (clean regions, depth, etc), errors can be different. I think we can set them as 0 as default and they are only effective when debiasing is True.

[confluence]

We're trying to eliminate the boolean parameters and deduce them from the options. So in this case, we need to decide if we need both of the errors to be provided, or just one. Will it ever make sense to use one error value and one default error of zero, or are both errors always required to do anything meaningful?

[kswang1029]

We're trying to eliminate the boolean parameters and deduce them from the options. So in this case, we need to decide if we need both of the errors to be provided, or just one. Will it ever make sense to use one error value and one default error of zero, or are both errors always required to do anything meaningful?

We need both to be provided if debiasing is True. If one is missing, we should throw an error.

[confluence]

OK, but we don't want the user to have to duplicate information in the high-level API by enabling debiasing explicitly -- we want to deduce whether the user wants to enable debiasing from the debiasing options provided. So if both errors are always needed, it's fine to keep the defaults of None, and enable debiasing if both the error parameters are provided. This is in line with removing e.g. the flag that enables the threshold, and enabling the threshold automatically if a threshold value is provided.

If the user provides only one of the errors, we should disable debiasing, but also print a warning (since this is likely to be a mistake).

[I-Chenn]

I think that this split is OK for now. You can, however, eliminate the boolean parameters enabling or disabling certain features, because you can deduce them from the other parameters. Set pixel_averaging_enabled to True if pixel_averaging is not None, otherwise False, and so on (in this particular case, make sure you change the validation for pixel_averaging so that None is allowed). You can do the same for threshold.

@confluence, sorry for the late reply since I am a bit busy this evening. I will take a close look for all your comments tomorrow but would like to make a quick confirmation:

Is the modification in 32aae92 what you mean in the quotation above?

[confluence]

Is the modification in 32aae92 what you mean in the quotation above?

Yes, that's correct! I also suggest that you do the same for debiasing, but as discussed, it should be enabled if both the error parameters are not None. If only one error parameter is provided, the debiasing should be disabled, but a warning should be printed.

I'll also make a few review comments.

[confluence]

Some nitpicks about the phrasing in the docstring.

[confluence]

You can simplify this and the conditional below like this: pixel_averaging_enabled = pixel_averaging is not None.

[confluence]

Naming convention: qError and uError should be q_error and u_error.

[confluence]

When you mention literals like None in the code, use the ReST markup for code, which is double backticks:

blah blah ``None`` ...

This will format the word as code in the final Sphinx document.

[confluence]

This should also mention that the default is 4 (plus the units; not sure what those are).

[confluence]

I would change the last sentence to: "Set to None to disable pixel averaging."

[confluence]

I'd rephrase this as: "Enable fractional polarization intensity. By default absolute polarization intensity is used."

[confluence]

Both of these should mention that the default is the current image.

[confluence]

I would say: The threshold. By default the threshold is disabled.

(We should probably also mention the units here.)

(If the user doesn't want to enable the threshold, they are more likely to omit the parameter and fall back to the default than to pass in an explicit None.)

[confluence]

stoke -> Stokes, but Error -> error

Once the debiasing parameter is removed, I would revise each of these to be something like: "The Stokes _ error in [units]. Set both this and [other variable name] to enable debiasing. By default debiasing is disabled."

(Edited to swap sentence order in suggested wording and simplify it.)

[I-Chenn]

@kswang1029 @confluence
I have made a commit (cb56287) to make several modifications to configure_vector_overlay function following your comments. Nevertheless, I found some further problems and would like to discuss with you:

1. angular_source and intensity_source setting:
   These two parameters actually do not set "Current" as default. It depends on what kind of image you open; if an image with polarization info is opened, these two parameters are set to "Computed" as default automatically. See below:
   
   May I have some hint of setting up the default values of angular_source and intensity_source under such circumstance?

2. The management of the situation "q_error is set but u_error is not set" (or vise versa):
   I am now dealing with the situation with the code:
   
   But do I need to set debiasing to some None value rather than just print the message?

3. Intensity area in the Styling panel:
   These two input fields seem to automatically update themselves after we apply the vector overlay. I don't know if we need to fill them up manually in advance (perhaps for some circumstances) or we just keep it a None?
   

[confluence]

Minor followup comments; it's looking good!

[confluence]

Please use the logger for this message (you'll have to import it at the top; it's in .util -- see how it's used in e.g. session.py).

You could compact this using syntax similar to the previous lines (as shown below), but it does mean restating part of the same conditional, so I don't feel that strongly about it -- maybe the way it is now is clearer to read.

debiasing = q_error is not None and u_error is not None
if not debiasing and (q_error is not None or u_error is not None):
    logger.warning(...)

In the message, please swap "be both" -> "both be". I would also refer to the error variables by their descriptions rather than their variable names here ("The Stokes Q error and the Stokes U error must...")

[confluence]

"The debiasing..." should just be "Debiasing..." in the last sentence(s) here.

[confluence]

"in pixel" -> "in pixels"

[confluence]

(And it needs the default -- I think it's fine to put "The default is 4." as the middle sentence; you don't need to restate the units.)

[confluence]

Sorry; I selected the wrong lines somehow -- this is related to the pixel averaging description.

[confluence]

I made my previous comments before I saw your questions -- answers below.

1. Perhaps the default parameter should be None, and the default source determined in the function using the same criteria that the frontend uses. You can use the image functions for determining if particular polarizations are available. The way that this default is selected would then be described in the documentation for the parameter. Edit: I was overthinking this. You should be able to get the default that was already computed by the frontend using get_value.

2. The debiasing can't be undefined -- it's either on or off (and it should be off unless both errors are provided). I suggested how you could rewrite this conditional -- in that syntax, debiasing is already set to False unless both errors are not None. If you keep the current syntax, you should also set it to False explicitly when you print the warning.

3. It looks like these intensity defaults are computed from the file. I don't think that this is necessarily going to be a problem. There's a single action for setting both the min and the max. We're planning to have a single style function for calling this and a few other actions (although maybe we should split this up further). If the user doesn't provide any intensity parameters we don't have to call this action at all; if they provide both parameters then we override both at the same time; if they provide only one, we can fetch the other one using get_value so that we don't override it. The only thing I'm not sure about is whether those defaults will be populated correctly if we apply the overlay from the scripting interface -- we should test that to make sure.

[I-Chenn]

@confluence , I added set_vector_overlay_style, plot_vector_overlay, set_vector_overlay_visible, show_vector_overlay, hide_vector_overlay to image.py and made some modifications to configure_vector_overlay following your suggestions in 401652e.
I haven't dealt with the action that need to be taken if user set one of the input field for Intensity area of set_vector_overlay_style yet, but the function is workable with the default setting under my simple test.
Nevertheless, I also tested configure_vector_overlay, but there seems to be some error at the last line of that function (the call_action line). I haven't figured out what the problem is and will work on it tomorrow.
Please take a look at the parameter descriptions in set_vector_overlay_style and see if it's OK for you if it's convenient to you. Thank you very much.

[I-Chenn]

@confluence, all the descriptions for vector_overlay functions are optimized, and I also added the descripted default value for color and colormap for contour. Please take a look and see if it's OK for you.

[confluence]

Clearing my own review.

[confluence]

@kswang1029 this is now ready for review.

(I would like to merge this first, and finalise making the raster / vector overlay / contours interface consistent in #123.)