dhale / jtk

The Mines Java Toolkit
http://inside.mines.edu/~dhale/jtk/
Apache License 2.0
83 stars 56 forks source link

Next LogAxis attempt #18

Closed ericaddison closed 7 years ago

ericaddison commented 8 years ago

Here is the next iteration of the ability to switch graphical views to a log axis scale. This is now achieved through PointsView.setHScale(Scale) or PointsView.setVScale(Scale), where Scale is an Enum containing scale options Scale.LOG, Scale.LINEAR, and Scale.AUTO. This has been achieved by extending Projector to project logarithmically from v to u. A subclass of AxisTics was created to deal with painting log tics, called LogAxisTics.

There are currently three ways to set the axis scale:

PointsView.setHScale(Scale.LOG);  // set this PointsView scale to LOG
Tile.setVScale(Scale.LINEAR);        // set all the views in this and adjacent tiles to LINEAR
Projector.setScale(Scale.LOG);       // set this projector to LOG

The PointsView method sets its own projectors to the specified scale, as well as the Tile it belongs to, if not null. The Tile method sets its own views to the specified scale, as well as the adjacent Tiles. The Projector method is currently protected, and called by the PointsView method.

When new views are added to a Tile, for example with PlotPanel.addPoints(), if they have the default scale setting of Scale.AUTO, it will assume the scaling of the current Tile. If the new view has a definite set scale, then it will change the current scale of the Tile when it is added.

Here are some current issues or things still to address: 1) Axis labels not quite right when x1[] values are all negative. 2) Currently only implemented for PointsView. 3) Next step, I want to add the AxisScalable interface. 4) Maybe some clunky adjacent Tile checking in the PlotPanel.addPoints() method 5) Currently negative x2[] values are drawn at the bottom of a plot when VScale is set to LOG ... might want to change to draw distinct line segments?

Ready for advice and criticisms!

image

ericaddison commented 8 years ago

I should also mention that LogAxisTics is not smart in the way that AxisTics is for determining how many tics to draw ... it always draws all the powers of 10 in the range of x1[] as major tics, and draws all 8 tics inbetween the major tics as minor tics.

dhale commented 8 years ago

I took another quick look, but there is a lot here and my mind is now too far away on other things for me to give it due diligence. Perhaps another Mines JTK user can build and test this branch.

chrisengelsma commented 8 years ago

Just a couple things:

ericaddison commented 8 years ago

Thanks for the feedback, Chris! I'll get those things taken care of.

I haven't written a lot of unit tests ... are you thinking of tests for the log scale projector and transcaler, or something more?

Thanks again!

ericaddison commented 8 years ago

OK Chris, I think I've filled in the unit tests and javadoc, and removed the stray debug code you found. I'm not sure what the best approach would be for automatically deciding whether a plot should be displayed in log scale ... maybe something about the range of x2[]?

Thanks!

@chrisengelsma

chrisengelsma commented 8 years ago

@ericaddison To be honest, it's something that can be kicked on down the road. I'm sure you could come up with something but personally "AUTO" being synonymous with "LINEAR" seems like fair logic to me. I will clone this and play with it very soon

chrisengelsma commented 8 years ago

@dhale i'm cool with it if you wanna take a look or merge

dhale commented 8 years ago

I remain preoccupied but did take a quick look this morning.

I see more changes than feel necessary. In particular, Scale.AUTO seems fragile, because it requires that linear/log state be kept consistent among instances of different classes. I wonder if this requirement is actually met, that any inconsistencies are well resolved.

What makes linear/log state different from other state maintained in Projector? Only the class Projector should maintain the state of log or linear scales. Other classes set or get Projectors. PlotPanel, for example, sets best Projectors in convenience methods like setLimits, and could do so as well in a method setScales. PlotPanel does not maintain its own state for limits, but by doing this in PlotPanel, we ensure that limits are set consistently for all Tiles in a row or column of a Mosaic.

What happens when Projectors with different scales are merged? In the current implementation of Projector.merge, this Projector always wins when the specified Projector p is inconsistent in some way. To be consistent, the linear/log scale of this Projector should win if inconsistent with Projector p.

What is the impact of these changes on performance of a PointsView with linear scales?

The class name AxisScalableTile is misleading because instances are not Tiles. Perhaps this class is unnecessary if we confine linear/log state to Projector only.

Should LOG be LOG10? I can imagine someday wanting LOG2.

Need to look for places in the current documentation that would be misleading if we add log scales. I'll bet that there are docs and/or comments where linear scales are implied.

No tabs. Indentation with spaces and whitespace around operators, function arguments, etc. should be consistent with other classes in the package mosaic.

ericaddison commented 8 years ago

@dhale Thanks for the comments, Dave. I agree with your points, and will see about pulling the scale state completely into Projector. Shouldn't be a big ordeal.

Currently in Projector.merge the Scale type is not considered during the merging until the computeShiftAndScales() method, where it switches to LINEAR if any part of the Projector range is negative. This was a behavior choice (what happens when a LOG Projector receives negative range?), and if you think it should behave differently then let me know. Other than that the merged Projector (this) will maintain it's original Scale.

Sorry about the tabs, I though I caught all of those! I'll fix up the formatting. Do you happen to have an eclipse format file?

Thanks!

dhale commented 8 years ago

Certainly, a log scale is valid only for positive values v0 and v1, and this seems like a good case for another Check.argument in the constructor. In other words, I cannot think of a case where users would expect us to silently switch to linear when log was specified.

The method merge is all about resolving conflicts between Projectors, and here we must choose a scale that will yield a valid Projector. I would not put this code in computeShiftsAndScales, which assumes a valid Projector.

Remove System.out.println's before pushing.

ericaddison commented 8 years ago

@dhale I was wrong about one of my previous comments ... I had set up PointsView to deal with negative values by just ignoring them if the range is partially negative, or switching to LINEAR if completely negative. I agree that the silent switch to LINEAR could be removed, but the ignore-negative-values behavior is similar to MATLAB, which may be familiar to users.

The logic to ignore negative values currently lives in PointsView.updateBestProjectors(), which constructs new Projectors with only positive values. This could probably be moved to Tile.updateBestProjectors(), but couldn't really go into Projector. This would move that responsibility out of the hands of individual views, which would relegate it back to their Tile, which would work with the Projectors to set up the axes.

How does that sound?

dhale commented 8 years ago

I like your current logic, having PointsView be responsible for constructing a valid best Projector, because I can imagine that other classes extending TiledView and supporting log scales might handle the log-of-negative-values problem differently.

When merging (inside Projector.merge) two best Projectors having different (log and linear) scales, what do you think of always choosing linear?

What to do when someone calls Tile.set*Projector with a non-linear Projector? We certainly cannot force all TiledViews to handle non-linear Projectors. So if any of the TiledViews in a Tile has a linear best Projector, then Tile.updateBestProjectors should give all TiledViews linear Projectors. In this way, Tile need only call (a new method) Projector.isLinear(). It need not know what other scales might be possible.

ericaddison commented 8 years ago

I like the idea of defaulting to LINEARin Projector.merge() in the case of conflicts, so that if you add a new view which is explicitly set to LINEAR to a Tile, then all connected views in the Mosaic will revert to LINEAR. However, I think that if a view is added with scale AUTO (which I think should be the default), then it should assume whatever the current Tile scale happens to be, or LINEAR if it's the first view. In any case, I think all views in a Tile should have the same scale of Projector.

This should also be the case if a TiledView is added that does not implement AxisScalable (set all to LINEAR). A similar check should be performed in the Tile.set*Projector() methods as you say.

dhale commented 8 years ago

The following seems sufficient and easier to understand for everyone: Linear best Projectors are the default for all TiledViews. So no AUTO. And no interface AxisScalable.

Mosaic appearance should be independent of the order in which users set Projectors, either directly on a Tile or indirectly via parameters set for a TiledView, which affect it's best Projectors.

ericaddison commented 8 years ago

OK, no AUTO, no interface. Will rely on TiledView base class to inherit scale related methods, with LINEAR as default for all new views.

For the situation of multiple views in a Tile, should we expect the same behavior from setting the scale via the Tile set methods as setting one of the views in the Tile? I would think yes, and in either case all other views in the Tile should be set to the new scale, unless one of the views does not support the proposed scale, in which case all views will revert to LINEAR. Is that reasonable?

dhale commented 8 years ago

I assume you mean to add something like TiledView.supportsScale(Scale scale), so that a Tile can determine (in Tile.updateBestProjectors) whether or not it can use a non-linear scale specified directly in a best Projector for that Tile. If this method returns false for any TiledView, then the Tile will use a linear scale. The implementation of supportsScale in TiledView would return false for any Scale except linear. Subclasses of TiledView that support non-linear scales would override this method.

Alternatively, Tile.updateBestProjectors would simply use a non-linear scale if and only if the best Projectors of all TiledViews in this Tile have the same non-linear scale. In this alternative, Tile would simply ignore the scale attribute for any Projector specified directly via Tile.setBest_Projector. I like this alternative because methods for setting limits in PlotPanel currently call Tile.setBest_Projector with Projectors having linear scales.

These methods for setting limits in PlotPanel are currently the only places within the Mines JTK where Tile.setBest_Projector is called. And I will guess that they are not called elsewhere. So we might also consider replacing these public methods in Tile with methods like those in PlotPanel that set only limits. To be safe, we should first @deprecate Tile.setBest_Projector, while adding new methods like Tile.setLimits.

ericaddison commented 8 years ago

@dhale OK, here are some significant changes made. First let me address the issues that you raised recently:

1) Removed or @Deprecated instances of setBest*Projector() in both Tile and PlotPanel in favor of new Tile.setLimits() methods. The axis scale does not change based on the setLimits() method, so if negative limits are requested for a logarithmic projector, then a Check.arguments() exception will be thrown.

2) When attempting to set a new scale for a Tile, the tile calls setScales() for every TiledView it contains. If the resulting scales of the views are not all equal to the requested scale (i.e. one or more of the views does not support non-linear scales), then all are reset to LINEAR.

3) Based on where a scale change is requested, either in Tile or through PointsView, the change will be propagated to all views and tiles in the Mosaic connected to the calling tile/view, resetting to LINEAR if at any time a non-compatible view is found. As a consequence of this, the scale cannot be set for a view or tile in a mosaic until all tiles are populated.

4) All scale state information is stored in the BestProjectors of Views and Tiles. Transcaler has an AxisScale field that is only used for temporary storage when calling combineWith().

After a PlotPanel is set up, there are now three basic ways to set axis scales:

PointsView.setScales(hscale,vscale);
PointsView.setBestProjectors(bhp,bvp); // protected method
Tile.setScales(hscale,vscale);

Plus a set of overloaded and convenience methods to access these. If desired, setScale() methods could be added to Mosaic and PlotPanel, which would set scale for a given row, column, or pair (irow,icol).

image

dhale commented 8 years ago

Quick response to your comments is all I have time for now. I have not had a chance to study the code.

Remove the convenience method Tile.setScales and perhaps most of messy problems/cases you cite go away. Here is my reasoning.

Projector constructor should Check.argument to ensure that a log scale is not specified with negative limits. I think this is what you meant above, but I was unsure who you have calling Check.argument.

Methods Tile.setLimits make sense because any TiledView must be able to honor limits (even negative limits) and support linear scales. So whenever a TiledView is given a Projector with negative limits, it will never have a logarithmic scale. TiledViews never assume they will be given their best Projectors.

Methods Tile.setScales do not make sense, because a Tile can handle any sort of TiledView, and not all TiledViews can be expected to support nonlinear scales.

Same thinking applies to any convenience methods in Mosaic and/or PlotPanel.

ericaddison commented 8 years ago

I'm gonna rehash how the scale change works now, and then see if I understand what you are suggesting.

As it is now, a Tile kind of manages the scales of the views it contains. If you call setScale() on a Tile, then it will attempt to set that scale for all views it contains (which ultimately means create a new BestProjector), and then pass the new scales up to mosaic, who then does the same for the other tiles in the same row/column as the original tile. If you call setScale() for an individual TiledView (only PointsView at the moment, others are No-ops), the same things happen if the view has a Tile; it still tries to set all connected views to have the same scale. If you give a TiledView a new Projector explicitly, it will still propagate up to the Tile and then Mosaic (unless specifically requested otherwise with a new boolean argument).

I have removed the public ability to give a Tile a projector, and replaced that with a setLimits() method. In this method, Tile creates it's own new projector with the new limits and it will take in whatever the current scale is via alignProjectors(). This is the meaning I took when you suggested removing setBestProjector() from Tile in exchange for methods that take only limits. This leaves open the possibility of a Tile having a LOG projector but receiving a request for negative limits, which currently throws an exception (Tile.setLimits( ) does a Check.argument), but could go back to the silent-switch-to-LINEAR behavior. Looking back at your comment maybe this is what you meant ... do you think that when calling setLimits() for a Tile, the scale should be set to LINEAR in any case? This would remove the possibility of trying to set a LOG projector to have negative limits. Additionally, Projector does a Check.argument() whenever its scale is changed, including in the constructor.

I don't quite understand why you say that Tile.setScale() doesn't make sense. Based on the current implementation, a Tile can handle any sort of TiledView, yes, but the Tile also requires that all of it's TiledViews have the same scale. If one of the TiledViews refuses to set to a particular scale (i.e. not supported), then Tile will set them all back to LINEAR, maintaining the same-scale requirement.

From your suggestions, and there's a good chance I'm misinterpreting this one, Tile would never worry about explicitly changing the scale of a TiledView, but would always return a LINEAR best projector unless the best projectors of all the contained TiledViews have the same non-linear scale (would also need to reach out to adjacent Tiles in the Mosaic for this check). With this scheme, a user would need to explicitly set the scale of all the TiledViews in this Tile and any adjacent Tiles to LOG10 before you would actually get a logarithmic display. Is that what you are thinking?

Thanks for the comments! I think we are converging on a solution here.

dhale commented 8 years ago

To quickly answer your last question, yes. Users who want a non-linear scale must specify a consistent non-linear scale when configuring their TiledViews. This requirement is easy to implement, easy to document, and creates no must-set-this-before-that problems.

Tile gives its TiledViews a Projector obtained by merging their best Projectors, accounting for any limits specified for the Tile. In cases where a scale is inconsistent with another scale, or with limits, Projector.merge yields linear.

ericaddison commented 8 years ago

OK, so if I understand right then the steps for a log-scale view might go something like:

// make a plotpanel
PlotPanel plot = new PlotPanel(a,b);

// set up views
PointsView pv1 = plot.addPoints(a1,b1,x1,y1);
PointsView pv2 = plot.addPoints(a2,b2,x2,y2);
... add more views ...

// set required views to LOG10
pv1.setScales(AxisScale.LOG10);
pv2.setScales(AxisScale.LOG10);
... same for all necessary views ...

That last step of setting all appropriate views to LOG10 seems like it could be easily automated by a convenience method setScales() in Tile, PlotPanel, and/or Mosaic... If there is a TiledView in the list of views that does not support the requested scale, then I don't see a difference whether that issue is caught explicitly by the user's code (as above), or by a convenience method ... I think it should just reduce the lines of code that a user needs to write if there happen to be lots of affected views... Further, it might get a little confusing or cumbersome for the user to decide which axes of which views need to be set to which scale in order to reproduce the 4-tile plot above. For example, above I would need to set the scales of 6 total TiledViews: two to (LOG10,LOG10), two to (LINEAR,LOG10), and two to (LOG10,LINEAR). With a proper setScales() method in Tile (or elsewhere), I could just call something like, for example, plot.setScales(1,0,LOG10,LOG10), and the rest would be taken care of automatically. This would still result in LINEAR best projectors if any of the views affected did not support the requested scale.

(that reminds me, I need to protect against explicitly setting a non-implemented TiledView with a non-linear projector through setBestProjectors())

Thoughts about this?

ericaddison commented 8 years ago

I guess I can see your point better with this kind of flow:

// make a plotpanel
PlotPanel plot = new PlotPanel(a,b);

// set up views one at a time
PointsView pv1 = plot.addPoints(a1,b1,x1,y1);
pv1.setScales(AxisScale.LOG10);
pv1.setOtherAttributes()
...

PointsView pv2 = plot.addPoints(a2,b2,x2,y2);
pv2.setScales(AxisScale.LOG10);
pv2.setOtherAttributes()
...

... etc ...

But still would require thought and potential confusion for mutli-tile plots I think...

dhale commented 8 years ago

Your PlotPanel with four tiles is good for illustrating what is possible, but is likely to be atypical and insufficient to justify increased complexity in the Tile-TiledView negotiation of best Projectors. In our support for non-linear scales, let's begin with the changes that are essential and easiest to document.

By the way, with Tile.set*Projector deprecated, we may want methods Tile.setMargins to go with Tile.setLimits. These margins (fractions of tile width or height) would then be used when computing u0 and u1 in a Tile's best Projector. Like limits, margins are another aspect of Projectors that all TiledViews can handle.

ericaddison commented 8 years ago

Gotcha. I will pull out any scale setting done by Tile and Mosaic, and return LINEAR best projectors from Tile if the scales of all TiledViews in the Tile do not match.

ericaddison commented 8 years ago

I have made most of the changes mentioned above. Tile and Mosaic no longer try to set any scales, but will return LINEAR best projectors if all relevant TiledViews do not agree on the same scale. This allowed a decent amount of code simplification with the whole scale-setting process.

There are now two demos. The first, LogAxisPlotDemo, plots a single Tile and sets the scales to LOG10. There are buttons to change the scales of each PointsView individually, to test the new Tile.setLimits() method, and to add a GridView, which does not have any scale stuff implemented (this shows what happens when an unsupported view is added to a Tile).

The second, LogAxisPlotDemo2, displays the 4-Tile Mosaic again with a single PointsView per Tile. There are now ComboBoxes in the window to set the Scales of each of the views individually, to explore the right combinations of scales for scale displays.

I haven't looked at the setMargins() stuff yet. I'll study up on that.

image

image

image

dhale commented 8 years ago

I've not forgotten this, and your screenshots look great. Just busy with other programming before traveling to the conference championships in Grand Junction for Mines Swimming this week.

Would anyone following this thread like to review Eric's pull request?

chrisengelsma commented 7 years ago

I'm pulling this out of stasis and taking a look at it

ericaddison commented 7 years ago

:) This was from more than a year ago? That was fast...

ericaddison commented 7 years ago

Cool! Thanks!