search

openmc-dev / plotter

Native plotting GUI for model design and verification
MIT License
48 stars 18 forks source link

Separate model view from view settings for more reliable reload #99

Closed kkiesling closed 2 years ago

kkiesling commented 2 years ago

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 commented 2 years ago

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.

kkiesling commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

kkiesling commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.