ankrh / MCmatlab

Numerical simulation tool for Monte Carlo propagation of light in turbid media
GNU General Public License v3.0
52 stars 14 forks source link

Moving sliders on Temperature evolution plot changes temperature limits #25

Closed TZ387 closed 2 months ago

TZ387 commented 3 months ago

Description of the bug:

Hi Anders,

When performing heat simulations, I had some problems related to temperature limits on temperature evolution plot.

The bug can be observed when running Example4_BloodVessel.m. In this code, the model.HS.plotTempLimits property is set to [37 100]. Initially, when the temperature evolution plot starts, these limits are correctly displayed:

However, when we move the slider (a similar but slightly different problem also occurs when checking the "log plot" checkbox), the temperature limits change:

Reason for the bug:

Upon examining the code, I noticed that the issue is likely related to the matlab files NdimSliderPlot.m and simulateHeatDistribution.m. After line 190 in simulateHeatDistribution.m, the NdimSliderPlot function is called as follows:

heatsimFigure = MCmatlab.NdimSliderPlot(model.HS.T,...
  'nFig',21,...
  'axisValues',{G.x,G.y,G.z},...
  'axisLabels',{'x [cm]','y [cm]','z [cm]','Temperature evolution'},...
  'linColormap',hot(256),...
  'axisEqual',true,...
  'reversedAxes',3,...
  'slicePositions',model.HS.slicePositions,...
  'docked',~model.HS.makeMovie);

Additionally, in line 201, the temperature limits of this plot are set with

caxis(model.HS.plotTempLimits); % User-defined color scale limits

While this code successfully sets the temperature evolution plot to the specified limits, a change occurs when we move the slider plot. This is likely due to the fact that when we run the MCmatlab.NdimSliderPlot function, the temperature limits are determined using the code starting from line 157:

%% Do the plotting
maxelement = max(data(:));
if p.Results.fromZero
  minelement = 0;
else
  minelement = min(data(:));
end
if maxelement == minelement
  maxelement = minelement + 1;
end

Although this behavior is desired for most plots (where limits are automatically calculated), it modifies the temperature limits whenever a change is made in the temperature evolution plot (as minelement and maxelement variables are passed to the NdimSliderPlotRedraw function).

Specifically, since we launched the NdimSliderPlot function at the start of the heat simulation with an initial temperature of 37 in Example4_BloodVessel.m, the minelement becomes 37, while the value of maxelement becomes 38. This is why the temperature limits in the image above were changed to [37, 38].

A possible solution:

This problem can be probably solved in many different ways. However, playing with my Fork (bug-temp-lim-fix branch), I tried the following solution:

In this solution (in NdimSliderPlot.m file), minelement and maxelement are enabled as two additional, "Parse optional" Name,Value pairs. While this slightly complicates the code, I think it would be beneficial since it would allow us to also specify limits (if desired) on other, non-temperature plots.

Here are the changes I made:

  1. In NdimSliderPlot.m, I added the following lines to parse the optional minelement and maxelement parameters:

    addOptional(p,'maxelement',[],@(x)isnumeric(x)); % If set, will specify the upper limit of the plot
    addOptional(p,'minelement',[],@(x)isnumeric(x)); % If set, will specify the lower limit of the plot

    Then, I would change the part of code that specified minelement and max element

    %% Do the plotting
    if isempty(p.Results.maxelement) || isempty(p.Results.minelement)
        maxelement = max(data(:));
        if p.Results.fromZero
            minelement = 0;
        else
            minelement = min(data(:));
        end
        if maxelement == minelement
            maxelement = minelement + 1;
        end
    else
        maxelement = p.Results.maxelement;
        minelement = p.Results.minelement;
    end

    If minelement and maxelement are already set, this code would take those properties, otherwise, it would determine them automatically with the code above. This part of the code can be further expanded if desired, f.e., it can also check whether either of the two element is NaN (not a number) for the case where we dont specify HS.plotTempLimits at all.

  2. In simulateHeatDistribution.m, I would modify the call to NdimSliderPlot function so that it would now include the specified limits:

    heatsimFigure = MCmatlab.NdimSliderPlot(model.HS.T,...
      'nFig',21,...
      'axisValues',{G.x,G.y,G.z},...
      'axisLabels',{'x [cm]','y [cm]','z [cm]','Temperature evolution'},...
      'linColormap',hot(256),...
      'axisEqual',true,...
      'reversedAxes',3,...
      'slicePositions',model.HS.slicePositions,...
      'docked',~model.HS.makeMovie,...
      'maxelement',model.HS.plotTempLimits(2),...
      'minelement',model.HS.plotTempLimits(1));

    Additionaly, the call

    % caxis(model.HS.plotTempLimits); % User-defined color scale limits

    is now commented out since it is not needed anymore.

  3. Finally, I would perform slight changes to heatSimulation.m , in which we insert the specified temperature limits (as part of the initial model class):

    plotTempLimits (1,2) {mustBeAllFiniteOrAllNaN, mustBeIncreasing} = [NaN NaN] % [deg C] Expected range of temperatures, used only for setting the color scale in the plot

    In that file, I added the validator mustBeIncreasing:

    function mustBeIncreasing(value)
      if ~isempty(value) && value(2) <= value(1)
          error('The second element must be larger than the first element.')
      end
    end

    This validator checks whether second element in plotTempLimits is larger than the first one. Additionally, it should throw an error if only one value is set

Again, there are other options to solve this bug, and I also probably missed some things, which is why I would like to hear your opinion on the matter.

Best regards, Tilen

ankrh commented 3 months ago

Hi Tilen

Thanks for the bug report. They are very valuable for us.

I guess I never got around to properly implementing robust color scale limits for NDimSliderPlot. I have now implemented something similar to your suggestion. Please let me know if this works for you.

-Anders

TZ387 commented 3 months ago

Hi Anders,

Thanks for fixing the bug. From my brief testing, everything seems to work as intended. Therefore, if I don't encounter any issues in the following days, I'll go ahead and close this topic.

Best regards, Tilen

p.s.: Now I noticed that while the temperature evolution plot is performing, clicking on the log plot checkbox changes the temperature limits to those supposed for linear plots. After brief checkup, my guess for this behavior would be updateTempPlot(h_f,M) in simulateHeatDistribution (caxis line) having same behavior for both linear and logarithmic plots:

if ~isnan(v.plotLimits(1)) 
  plotLimLower = v.plotLimits(1);
else
  plotLimLower = min(h_f.UserData(:));
end
if ~isnan(v.plotLimits(2)) 
  plotLimUpper = v.plotLimits(2);
else
  plotLimUpper = max(h_f.UserData(:));
end
if plotLimUpper <= plotLimLower
  plotLimUpper = plotLimLower + 1;
end
h_ax = v.h_surfxslice.Parent;
caxis(h_ax,[plotLimLower plotLimUpper]);

drawnow;