Masking data

[nguy]

This is a project documentation noting I would like to implement a masking feature as a plugin.
It should:

• Mask data inside a chosen region
• Mask data outside a chosen region
• Be the basis for an editor feature of the data
• Have an interface where a field and value(s) may be chosen to mask data
• Have the ability to save the data to a new output file

Much of the masking can be done using a Pyart Gatefilter.

The Region of Interest plugin can be connected and a commit is proposed to make this possible.

Discussions of interest are in:
PR #56 and Issue #25

[nguy]

@gamaanderson @tjlang @pfhein I added a gatefilter plugin to a branch:
https://github.com/nguy/artview/tree/ng-gatefilter

I'm have not solved the following issues:

1. Assigning a parent to new window (e.g. MainMenu).
2. Getting the Displays to update following application of filter.
3. Updating Vradar when variable is changed in first Combobox in menu window.

Any insights are welcome.

I will be without internet this weekend.

[nguy]

Oh and please comment on the interface. I tried couple of slightly different things.
Listing only Radar variables at the top and Showing the code that is used that could be used in a batch script.

[gamaanderson]

for your points

1. the only one without parent is the new Display, your passing None to it.
2. I think you miss understood how GateFilter work, it does not change the Radar, it just create a mask (bool array of shape (nrays,ngates)) plot that is not applied any where. As it is now, is not that the Display is not updating, it is plotting exactly the data that is being passed to it. To solve that there are two options: first change the data permanently in the Radar, the Display plots the changed Radar as any other Radar. Second: we change in PyArt to make pyart:RadarDisplay accept a gatefilter and make VgateFilter a shared variable of artview:RadarDisplay that can be connected to the plug-in. I prefer the second choose, also because it helps the community as we are improving pyart.
3. you are not connecting self.radarCombo to anything, so it is not change anything. I do however prefer to have Vradar as a shared variable, that way you let artview control (link/unlink) that for you. That part of artview is made to remove this kind of work from the programer, making it easier to build plug-ins.

Now my points:

1. You probably know that, but I am saying anyway. I really do not like starting a new display window, not only from the programer side that I already explained, but also from the user side, I really don't like some kind of radar programs that in order to visualize something new, it pop-up a new windows. You end up with a lot of window that you did not ask for. Also one does not need it, the other display are visualizing the same radar, it just happened that as radar was changed with out letting them know (i.e. not executing .change()) they didn't realize the new field. A unlinking is enough to wake the Display for that.
2. I may be wrong, but I don't see how the drop-down Variable selection is better them the TreeView of ChooseVariable.
3. This is a personal preference, but instead of the drop-down of operations I prefer all operations to have they own line. It does take more space, but make easier to visualize and understand what is happening, as every line is like executing a function from the GateFilter class. I like graphical interfaces that are closer to the programming language, notice that everything we do could be done directly in a python terminal with pyart, our interface is just more convenient.
4. You may want to add a way of changing the field.

[nguy]

Okay, I was under the impression that the mask was applied to the data because of the matplotlib interface, and in fact the Matplotlib page says that the mask is applied when plotted. And in fact I've used this in other applications with success. Is this correct @jjhelmus?

I could not find the parent information. But you may make a good point in not opening an additional window and doing the application to the current window. This could be changed.

The reason I like the Variable selection is that I think it is easier for the user to only be given the choice to choose a Variable. As it is now they won't necessarily choose a Variable instance that works, I'm not sure we can expect someone to know the code well enough to choose properly.

I see your point about the drop-down of operations, but that is also why the ShowScript button is there, so that the user can see what operations they explicitly performed. Also, I think it is cleaner to choose an operation for a field, rather than vice versa. I couldn't think of an instance where I'd want to use the same field twice, but I could think of many instances where the operation would be used more than once. Make sense?

[nguy]

@gamaanderson You mentioned that you didn't like to bring in Display instances like we did in ROI, but this will be needed for GateFilter as well, I think. I don't see a way around that.

[gamaanderson]

@nguy I strongly disagree, I don't see any reason for this close relation Display-GateFilter. There is a reason for ROI need Display, the selection mechanism explicitly need to happen in a Display. GateFilter is pure data manipulation, there is no need for a display. What I do expect, is an alteration in Display to add a shared variable VgateFilter.
Continue working, unless you do some thing really out of the expected, I'm sure we can later strip Display out of it.

[nguy]

I see what you are saying I think. We should be able to use the GateFilter and update the Vradar which will be applied to the already existing display (just as the additional corrected fields show up now). Is there need for a VgateFilter? I guess this goes back to whether the display should update with an updated mask.

The approach I took was to:

1. Duplicate fields with the added "corr_" onto the front of the field name.
2. Select the exclusions via the interface.
3. Apply the exclusions to the mask of the corr_field variable.
4. Update the plot.
5. The user can then decide whether to save these fields into a cfradial file and/or look at the commands needed to perform this and create a batch script.

[gamaanderson]

that sounds right and in fact in this approach there is not need for VgateFilter.

[nguy]

I just made a quick update to a PyArt branch that adds pyart.correct.GateFilter as an keyword that can be used in RadarDisplay. If this change is accepted, we'll need think about how to add gatefilter into ARTView to be passed.

[gamaanderson]

Nice, adding gatefilter to display should be quite straight forward with shared variables. I will make the needed alterations so that you can already use artview in your pyart branch.

[gamaanderson]

It is in this branch, this should be combined with yours but this outdated, GateFIlter is now in pyart.filter, not pyart.correct.

[nguy]

@gamaanderson I updated the gatefilter branch. It looks as though Vgatefilter is changing value, but the Vradar mask doesn't seem to be updating.
Any thoughts on where my error lies?

[nguy]

I've been staring at this for a bit now, trying a couple different things. For some reason the display does not update even though the Vgatefilter instance is changed.

[gamaanderson]

For me it worked just fine, I just notice that Vgatefilter from Display and from GateFilter plugins are not linked by default, which is pretty much the wanted behavior since it has a simple startupGUI.

[nguy]

Weird, it is not working on my end. I thought that by doing the self.sharedVariables() and then self.connectAllVariables() this linked the variables. Is that nor correct?

[gamaanderson]

they 'link', but which one is 'linked'? the one you passed to __init__. In the case of GateFilter it is a simple initiation, so variable is been passed.

[nguy]

Okay, I think I see. It's initiated, but only with the variable. I have to link the display and GateFilter manually through the LinkPlugins window tool.

Is there a good way to programmatically link the two once the Vgatefilter is initialized?

[gamaanderson]

A good, good way, there is not, but is can be developed in the guistartGUI. Some components, like display, have specific guiStart. Developing that is the way to go, for instance with a drop-down box.

[nguy]

Do I bypass this with the chooseRadar function at the top?

And just so I get this right (since I mimicked your code to get it working). The MenuRadar instance is the one being operated against (as selection 0) and which has been updated by other components. However no update to any displays are gained until the manual link is done.

What do you think if I update the chooseRadar to be chooseDisplay? Then the user must choose a Display instance that will then cause the program to apply the the link and refresh the chosen display.

[nguy]

Okay I updated the chooseRadar to chooseDisplay. It looks for any components that have a "Vradar" and "Vgatefilter" shared variable and list only these choices. It auto loads the first to be default and will operate on this instance.

I could see argument for this being in the guistart also and this way the user can choose different displays if they would like.

[nguy]

@pfhein @tjlang Can you take a look at the newest version of this branch and let me know your impressions of the GateFilter.

Caveats are that it's the mask that you are updating for the plots. And when you do this you update every the mask for every field. Individual fields may be possible, but it would take some work. Any time you would use this without filtering all fields?
The upside is that no data is actually changed. It's all an applied mask, leaving all data available at it's root.

So I think the filter field check boxes should be removed.

[gamaanderson]

in pyart gatefilter work over all fields, so I see no particular reason we shound't. Anyway the user can always open new Display with a different Vgatefilter. It would however be interesting to in the RadarDisplay turn the filter on/off.

[gamaanderson]

Aboult the change to chooseDisplay I don't particularly like it, for the simple reason that now it only works with displays. Other correction plugins can also use a Vgatefilter and that will have to be connected in the standard artview ways (now LinkPlugins, future drag-drop), and I can realy think in circumstances where chooseDisplay can become inconvenient, so you are changing versatility/usability for user experience.

Further more having individual components implementing their own linking tools and not using the standard ones may bring confusion.

I would really want users to understand what is a shared variable like I do and to know how to operate the LinkPlugins, then once you know what you are doing it is really practical and simple to use, I do it all the time. I do however understand that this wish of mine is probably unrealistic and insisting on it does more harm than good.

For last, sorry if my comments are some what annoying some times, I'm always pointing thinks I don't agree with. I realy like to improvements you are doing, I'm just pushing it the on side and it is up to you(s) to push to an other so we can get an equilibrium.

[nguy]

I agree it would be interesting to turn filter on/off in display. That could be an option in Display Options. Shouldn't be too difficult.

I struggled with whether to use the default LinkPlugins or not. Ultimately as you point out I sided on the user ease side. I can see the argument for using the LinkPlugins, but feel that users with less computer knowledge are going to be completely lost with this. The more advanced users could make this a powerful tool (and should though we probably need more documentation on this).
I think maybe we need to go into a bit more depth/more clarity with our shared variable explanation. Admittedly, I think I'm finally getting a hold on this. In short we need to make LinkPlugins more intuitive and maybe the drag-and-drop will do that.

Another option I see would be to put something like chooseDisplay and others as functions within Component for developer shortcut use. GateFilter only uses the two shared variables and the link must happen somehow for the displays to work.

No need to apologize for comments. I see criticisms as ways to make this code more solid.

[gamaanderson]

Ok, I understand what you say and agree with it.

A (not so) small note: as a tool for developer I putted chooseVariable in core, so from this point of view chooseDisplay should be in core. On the other hand, display is not a "core" part of artview, it is only a specific component, and therefor chooseDisplay should be in Component. I would probably opt for the second, as you suggested, but this kind of conflict for me is like a Warning sign that something is wrong with the idea. I can be wrong, but I believe that things like chooseDisplay are likely to grow in complexity and multiply.

I'm not really sure what to do now, so we probably shouldn’t change anything, just let this mental footnote.

[gamaanderson]

One more thing: in the end there is not formal problem, as plug-ins, different than standard components, are not subject to "code quality control", so is not an "error" to apply non-standard solutions for programing problems. We should just focus in making the artview programing tools the most useful and make good example using them.

If you could get this impression in the conference, I would like to know how interested people are in artview as a program and/or as a visualization library (like an extension of pyqt based in data view/manipulation). Artview is and should continue to be both, I just like to know what to focus in.

I also like to think of artview as a Windows Manager, it take tasks that can be done in text mode (with pyart) and give it a graphic mode. In this context the plugin AccessTerminal, is really like a console, going back to text.

[nguy]

Yes, you are right chooseDisplay could grow in complexity. So let's hold off changing the core functionality as you suggest. Future updates or additional plugins may make this more clear.

I am thinking that I may merge the GateFilter and think of this as the first version, as ROI eventually became a more robust SelectRegion.

[nguy]

Your thoughts are exactly some of the points I want to raise at the conference. I'm interested to see what people find most useful as well. I've received some limited feedback (pretty positive) on the display.

Also was told that the SelectRegion helped diagnose a problem. And I'm meeting with NCAR to discuss if this is a potential replacement for the Solo package (editing radar data files).

I really like the AccessTerminal and view you put in, those really elevate the flexibility of ARTView.

[pfhein]

@nguy - I would also remove the filter field check boxes. In the help, I would add an explanation of what inside and outside mean and when do you need to use value2. It is right now a little confusing, but I am excited to have that capability. Unfortunately I got an error KeyError: 'reflectivity'. I suspect it is the 2 letter named variables in the file I chose. (I need to move out of the 1980's UF variable names.)

You are really cutting edge. I had to update pyart to get the gatefilter stuff. Pyart was only a few weeks old.

[gamaanderson]

The optimal solution for what @pfhein pointed out is to make the value2 appear/disappear with the options. This will need little more code, so just putting a help may be a quick fix. I'm not sure how this help should appear, may be a help button giving a detailed explanation of the whole plugin. I forgot so say, but I also get the reflectivity error even with my pyart configured to DBZH, so I needed to get a file with the reflectivity field.

@pfhein We do pretend to let your pyart requirement a little late in relation to the ARM, but this alterations in pyart were done exactly with this plugin in mind, so it was unavoidable. This is however a point for not merging, you could just let it as a separated branch and inform people about it (like with a 'future' section on README). This could eventually create merging conflicts, but there is no reason for being afraid of them and if we work right they tend to be minimal.

[nguy]

Okay, I'll remove the filter check boxes and add some explanation text.

What 2 letter name are you using Paul? I could add that as well, it's an easy update.

The reason this is so cutting edge is that I had to update PyArt to make this work! :)

[gamaanderson]

@nguy the error happen here, I think this is just for debugging information, could it be removed?
I think the possible names for reflectivity in UF are DZ, CZ, ZT and ZE

[pfhein]

It error was for reflectivity which is DZ but the filtering was on CZ which is corrected reflectivity.
Here is the traceback:

Traceback (most recent call last):
  File "/home/hein/work/artview/gatefilter/lib/python2.7/site-packages/artview/plugins/gatefilter.py", line 511, in apply_filters
    print(np.sum(self.Vradar.value.fields['reflectivity']['data'].mask))
KeyError: 'reflectivity'

[nguy]

I'm thinking that merging is still the way to go for exactly the reason you say @gamaanderson that merging conflicts could become a bear later on. Plus our user base is small for now, so we take advantage of that. If a bug is submitted, it's easy to tell them to update to the latest.

Yep that is purely a debugging line, I'll remove those.

[gamaanderson]

conflicts are not as big a problem as most think, with the right tools it is quite easy, see git mergetools.

[nguy]

Will do!

I updated the code with I think all of the above suggestions. Except the value 2 disabling of text field. I am going to have to look into that one a little more. But I added text to the column labels that is a patch for now.

[nguy]

Okay, so I just also added a checkbox into the Display that will allow the user to apply the the gatefilter mask or remove. See the latest push.

[gamaanderson]

Why you didn't put the check button, in the display options? You had problems with it? Buttons can't go into menus, but actions can be chekable, you need them to connect the toggled signal.

[gamaanderson]

Also, some may disagree, but I would prefer to see the name GateFilter in this button, so that the user knows what is the underling pyart functionality.

[nguy]

I did it this way to make it more accessible without going into menus. But this could be changed easily.

[nguy]

I also wonder if more of these types of things will come up in the future where a series of "easy checks" will be of use?

[gamaanderson]

I prefer to keep things simple, so I probably wouldn’t like the display to become much more complex than it is now, we may add some options to make a better use of the options pyart provide, but I don't think much enhancement is to be done in this scene. Even if we decide to make a big enhancement to provide other kinds of visualization that could be done in a new component to let the RadarDisplay "light" .

[nguy]

Yeah, I see your point and like the interface also. Let me change to a button mode to see if I can get that working.

[nguy]

Okay, moved the selection into Display. Still having some issues with functionality. Vgatefilter is not being applied consistently.

[gamaanderson]

I would have gone with a simple checkable action, but your radio work just as well. I didn't understand what functionality problems are still there, it works just as fine under its proposal.

Just one comment, the save radar routine permanently add the filter to the radar, while this side effect is not a problem and probably my be wanted, this is not what the user expects. I mean, I would expect this filter be applied to the resulting file, but not the radar object.

P.S. pyart just released 1.5

[nguy]

It actually does work fine. Every so often, I get a display that is completely wiped out. But by turning off the GateFilter display it resets just fine.

I think that using the save file from GateFilter the user does know (I hope) that the mask underneath has been notified. But if they save from elsewhere, maybe we should throw up a warning.

[nguy]

For future reference, different fields do have different masks in some radar volumes. When using GateFilter, the same mask is applied using a logical 'or' statement on each field. Therefore, I don't think any side effect exists. But for posterity I want to note this behavior in case a problem is found and one of us reviews this interaction.

[nguy]

Oh and I forgot to note that the check was being difficult. This could be changed later, but I also found the current way to be very clear on whether GateFilter was applied.

[gamaanderson]

ok, if there is nothing more, I would be ok with merging. Also roll the version, as it is kind of big.

[pfhein]

Yes different fields can have different masks. That can cause problems. A year ago I was trying to get dealiasing working and had to combine masks to get things to work. The reflectivity field has gone through some processing and had a different mask. Here is my code that combined the masks and set the bad values.

# combine the masks  (noting two Falses is a good point)
    combine = np.ma.mask_or(radar.fields['AZ']['data'].mask,radar.fields['VR']['data'].mask)
    radar.fields['AZ']['data'].mask = np.ma.mask_or(combine, radar.fields['AZ']['data'].mask)
    radar.fields['VR']['data'].mask = np.ma.mask_or(combine,radar.fields['VR']['data'].mask)
    radar.fields['AZ']['data'].data[:]=np.where(combine,radar.fields['AZ']['_FillValue'],radar.fields['AZ']['data'].data)
    radar.fields['VR']['data'].data[:]=np.where(combine,radar.fields['VR']['_FillValue'],radar.fields['VR']['data'].data)

I also agree that a minor version number increase would be in order. I am okay with merging.

[nguy]

Thanks I'll create a pull request and merge the code and version.
Paul, I'm going to stick that code in there commented out for now and revisit this later if there are issues with the current method.