Separate model view from view settings for more reliable reload

[kkiesling]

I am making this a draft because I think it needs work, but it could use another set of eyes. I think this is a bit clunky the way I have it still.

The point of this change is to separate the viewer settings from the model settings such that when something is reopened, the view can still be reset even if the model has changed. To do that, I separated the PlotView class into two classes: PlotViewIndependent and PlotView. The latter inherits from the former which contains all the model-independent information for the view settings. When PlotViewIndependent is initialized, there is the option to use a past view (eg currentView from the plot_settings.pkl file and it will set all the parameters based on that view. And then PlotView will still use the cells and mats from the current load of the model.

This allows us to remove the checking of the model hashes when we want to load the plot_settings.pkl file because instead of loading all the previous information, we just initialize the current model right off the bat with the last view from that pkl file and do not worry about the cells, mats, etc. that could have changed. Those will still get loaded in from the current model.

Question: How important is it to reload the previousViews and subsequentViews from the plot_settings.pkl file? Right now with these changes I don't reload those lists only because the mat/cells could be changed from those views. I do have a workaround for that in mind, but I think it would be time consuming to implement for large models (I'll add what am thinking to an inline comment where it should go). If these things are not important, then we could also greatly reduce the size of the pkl file that is written out by only saving the currentView attributes that are necessary for reloading (and the statepoint file info).

FWIW- I have tested this and it does reliably work to reload changed models but keep the view settings. But it could probably be more elegant.

fixes #96

[kkiesling]

Alright, I can't actually think of how to not make this so clunky so I am opening it for review. Let me know if you have ideas for more streamlining.

[paulromano]

This is a step in the right direction, but currently misses the main benefit -- when you change settings, it still calls id_map and property_map even when it doesn't need to. The simplest example in my mind is switching from a color-by-cell view to a color-by-material view. We need some extra logic in PlotModel.makePlot to check whether the model-independent stuff changed. If it didn't change, there's no point in calling id_map and property_map as they will result in the same information being returned.

[kkiesling]

This commit is a very rough draft (ignore debugging print statements) of trying to assess whether the view has changed from the previous view. If there are no changes, where are we supposed to take the ids (cell, mat, etc) from? The only way I can think is to store the results of id_map and property_map as attributes somewhere.

[kkiesling]

I have a "working version" of checking the view settings in makePlot such that we don't have to make unnecessary calls to id_map and property_map. To do that, I actually separated the class out one further to isolate just the attributes that come from the _PlotBase class (new class is ViewParams. These attributes are the only attributes that need to be checked if they are the same because they are the only things necessary for id_map and property_map. Also to make this check work, I made ids and props class variables (__ids_map and __props_map) so that the info is there and only updated if changed.

BUT I put "working version" in quotes because yes this does work exactly as intended. However, I this small set of view_params attributes that matter for these function calls actually do change by just switching from view by material to cell. Like you @paulromano I did not expected changing from material to cell to cause any difference, but you can actually print the parameters with each change and see that they are different.

To check this out, I left some print statements in makePlot. Open the plotter with the test file. You should see the initial view setting print out. In the plotter, switch from material to cell view (but do not change anything else). When you hit apply you'll see the view information print out again and that the VRes is now different, just from changing from material to cell. That value is what defines the shape of what is returned from id_map and property_map, so that does actually need to be called again. I really don't know why this is, but it is.

Here's the output from the above test:

Open the plotter, this shows the initial view parameters:

>> openmc-plotter 
                                %%%%%%%%%%%%%%%
                           %%%%%%%%%%%%%%%%%%%%%%%%
                        %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
                      %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
                    %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
                   %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
                                    %%%%%%%%%%%%%%%%%%%%%%%%
                                     %%%%%%%%%%%%%%%%%%%%%%%%
                 ###############      %%%%%%%%%%%%%%%%%%%%%%%%
                ##################     %%%%%%%%%%%%%%%%%%%%%%%
                ###################     %%%%%%%%%%%%%%%%%%%%%%%
                ####################     %%%%%%%%%%%%%%%%%%%%%%
                #####################     %%%%%%%%%%%%%%%%%%%%%
                ######################     %%%%%%%%%%%%%%%%%%%%
                #######################     %%%%%%%%%%%%%%%%%%
                 #######################     %%%%%%%%%%%%%%%%%
                 ######################     %%%%%%%%%%%%%%%%%
                  ####################     %%%%%%%%%%%%%%%%%
                    #################     %%%%%%%%%%%%%%%%%
                     ###############     %%%%%%%%%%%%%%%%
                       ############     %%%%%%%%%%%%%%%
                          ########     %%%%%%%%%%%%%%
                                      %%%%%%%%%%%

                 | The OpenMC Monte Carlo Code
       Copyright | 2011-2022 MIT, UChicago Argonne LLC, and contributors
         License | https://docs.openmc.org/en/latest/license.html
         Version | 0.13.1-dev
        Git SHA1 | 9acaf37b9df9028c1fe2bd77cd786faaa998788c
       Date/Time | 2022-06-23 16:09:22

 Reading settings XML file...
 Reading cross sections XML file...
 Reading materials XML file...
 Reading geometry XML file...
 Reading U235 from /Users/egsuser/kkiesling/nuclear_data/endfb71_hdf5/U235.h5
 Minimum neutron data temperature: 0 K
 Maximum neutron data temperature: 1.7976931348623157e+308 K
 Preparing distributed cell instances...
 Reading plot XML file...
*** NEW CALL TO MAKEPLOT ***
True
Previous View Parameters
-----
Plot:
-----
Origin: (0.0, -2.5, 0.0)
Width: 20.099999999999998
Height: 15.075
Basis: xy
HRes: 1000
VRes: 1000
Color Overlaps: False
Level: -1
Current View Parameters
-----
Plot:
-----
Origin: (0.0, -2.5, 0.0)
Width: 20.099999999999998
Height: 15.075
Basis: xy
HRes: 1000
VRes: 1000
Color Overlaps: False
Level: -1
generating new id/prop map

Switch from view by material to view by cell:

*** NEW CALL TO MAKEPLOT ***
False
Previous View Parameters
-----
Plot:
-----
Origin: (0.0, -2.5, 0.0)
Width: 20.099999999999998
Height: 15.075
Basis: xy
HRes: 1000
VRes: 1000
Color Overlaps: False
Level: -1
Current View Parameters
-----
Plot:
-----
Origin: (0.0, -2.5, 0.0)
Width: 20.1
Height: 15.075
Basis: xy
HRes: 1000
VRes: 749
Color Overlaps: False
Level: -1
generating new id/prop map

The "Current View Parameters" are the new settings after the switch and they are different.

[kkiesling]

Although, as I continue to use that test file and just switch between cell and material view multiple times, it does actually appear to be fine (in that it doesn't change the view settings and therefore makes no extra calls). So it is maybe just an issue with the first view of the plot.

[kkiesling]

I think this is ready. I can't quite figure out the hres/vres issue we were seeing. I will keep poking around at it, but if any changes arise, I'll put them in a new PR.

[paulromano]

Is there any reason we can't just save the entire currentView? As is, this misses other information. For example, if you change cell/material color assignments, close the plotter, and re-open, those assignments are gone.

[kkiesling]

The goal of this was to prevent having to store the large arrays with the cell/mat/property data to the pickle file smaller. Plus the goal of this PR was to separate the model-dependent properties from model-independent properties. Though I can see how this property is in the gray zone. If the material/cells IDs change, then the mapping of colors to materials/is no longer valid. I will play with it and see what can be done. Might have to go back to using hashes in that if the hashes are unchanged, the entire current view can be reused, but if the hashes differ, then use only the independent view settings.

[kkiesling]

Ah, and I am just remembering that we stored the full model in the past which is where that large amount of excess data came from. So storing the full currentView should be no issue.

[pshriwise]

Thanks for taking this on @kkiesling. It's a long-awaited and non-trivial improvement. A few additional comments from me here.

I am still having some trouble loading default views and the views I get when using the "undo" feature don't seem quite right. Are you seeing the same?

[pshriwise]

Seems to me that incrementing the version number on the plot_settings.pkl file would allow us to differentiate between a version incompatibility and an error reading the settings file more easily.

[kkiesling]

That would work to some degree, yes, and the version should be incremented. But one issue is that data = pickle.load(file) fails before version number can be read from the pickle file. So some try/except might still be necessary.

[pshriwise]

Now that we're storing all of the ids as an attribute on this class, perhaps we should make the cell_ids, instances, and mat_ids properties of the PlotModel class so we're sure they're always consistent with the ids_map attribute.

[kkiesling]

Just to double check, these don't also need to have a settr defined right? I think it works to just set them as properties.

[pshriwise]

This looks like a pre-existing issue, but it now causes a failure when trying to reset to a default view. Same with v_res in the line above.

[kkiesling]

I was very confused by this block and wondering if it was a typo..

[kkiesling]

Okay changes have been addressed. I think defaultView gets reset properly now (@pshriwise double check that the error is not occurring for you anymore). And I added the hash checks back in to be able to check if things like material/cell colors can also be restored (if hashes are unchanged, then we can restore the domain information too)- @paulromano check to make sure it does what you want it to be doing too.

[kkiesling]

@pshriwise do you know why this was initially set to True? And is that the desired behavior? I ask because the way that this code is currently setup, by setting this to true it actually runs applyChanges() during creation and stores the this initial view as a previous view upon opening. This means that the "undo" button in the menu already has 1 undo possible without really having a view to undo.

[pshriwise]

Hrmm yeah that's not ideal and potentially confusing.

As for why this particular line is here, I think it's simply being set with an initial value like the other attributes. It's been a while, so I'm not sure why setting this particular value (as opposed to any of the others) would trigger a callback to applyChanges(). Can you elaborate how setting this to True triggers the applyChanges() call?

[kkiesling]

Setting it to True essentially "turns it on" as if it were a change in the toggle. The relevant lines are where it's added to the menu here and then the actual updating of the setting here. I don't fully understand why it behaves like this to be honest, other than it is caused by this being initialized to True. The other 3 settings in the same part of the menu that are setup the same way are initialized to False and do not trigger a change to applyChanges().

[pshriwise]

Got it. Thanks for the links! They helped me track down the issue.

I discovered that the flow which causes this is:

1. Model is loaded when initializing MainWindow
2. PlotIndependent.masking is set to True for the initial current and active view.
3. The GUI continues loading in MainWindow.loadGui and MainWindow.updateEditMenu is called
4. (The crux) MainWindow.updateEditMenu calls the setChecked method of the masking check box to ensure it's state matches that of the view. If this call changes the state of that check box, then the callback method is going to trigger an applyChanges call and create a new view.

It seems to me that the default value of the masking check box in the color dialog doesn't match the view setting and results in the creation of a new view.

I verified this by adding this line in MainWindow.createMenuBar

self.maskingAction.setChecked(True)

before the item is connected to its callback method. This removed the superfluous previous view for me.

I'd say that fix is sufficient for now, but in the future I think it would be nice to have some internal setting that prevents any updates while the state of these data structures is initialized would be good.

[pshriwise]

It does indeed look like this is pre-existing behavior. I'll make an issue so we can clean it up in a subsequent PR.

[kkiesling]

I figured out why the undo wasn't working- the key is to compare the active (desired view to update to) to the current view, rather than comparing the current and previous views. Should be working as desired now.

The only outstanding question is whether it is intentional for 'Enable Masking' to be on by default when a new instance opens? This is what causes the gui to have a view in the undo list when it is first loaded. It's easily fixed by just setting to the default masking to false in the PlotView initializing.

[paulromano]

I just played around with this for a bit and now all the behavior works as intended. This is a really great structural improvement -- thanks @kkiesling! Once @pshriwise is satisfied we can merge.

[pshriwise]

Tried it out too and it's working great! Thanks for these updates @kkiesling. It seems there's one remaining issue with an initial view change happening behind the scenes. If that's a pre-existing issue, then I'm fine w/ handling that in a separate PR.