Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VectorSeries #1672

Merged
merged 18 commits into from
Aug 16, 2023
Merged

VectorSeries #1672

merged 18 commits into from
Aug 16, 2023

Conversation

VisualMelon
Copy link
Contributor

Working on #107, putting this here so that anyone who wants to explain all the things wrong with it can do so, because I've not touched it recently and am making no progress, and would appreciate any feedback. There are also a couple of things in here I find myself wanting to point at on occasion.

Checklist

  • I have included examples or tests
  • I have updated the change log
  • I am listed in the CONTRIBUTORS file
  • I have cleaned up the commit history (use rebase and squash)

Changes proposed in this pull request:

  • Add a VectorSeries, which plots a series of VectorItems as arrows on an X/Y axis pair
    • VectorItems have an Origin, Direction, and Value (i.e. it's a vector with some arbitrary value positioned at a particular point)
    • The value is mapped to a color on a ColorAxis
  • Adds a DataVector struct
    • I would like to go through the existing code base and replace usage of DataPoint as a vector with DataVector, though I'm not sure there are any instances (I need to check)

I'm not entirely happy with rendering, but that might be because I'm never entirely happy.

It doesn't make plotting a vector field especially easy, because it just plots arrows. We could either provider helpers to map a vector field (i.e. 2D array of vectors) to a list of VectorItems, or we could make VectorItem optionally read from a multidimensional array, or we could provide a separate VectorFieldSeries to do that.

I opted for X/Y vector rather than Angle/Mag because the former would be confusing wrt. the plot aspect ratio: if a user wants angle/mag, then they should be fixing the aspect ratio. (Note: that isn't easy...)

Have I totally missed the point?

@oxyplot/admins

@Jonarw
Copy link
Member

Jonarw commented Oct 3, 2020

I'm not sure I can completely get behind the idea of defining the vectors using X/Y coordinates rather than Magnitude/Angle. This might have to do with the only kind of vector fields I ever really was in contact with - which is optical representations of magnetic/electric fields, something like here.

Here it doesn't make sense to specify the vector in terms of X/Y coordinates, because it represents a completely different thing: while the X/Y coordinates represent a point in 2-dimensional space, the vector length and direction represents the field strength and direction. Consequently it also doesn't make sense to change the direction or length of the vector when the X or Y axes are scaled. Also in this context it makes no sense for the vector to become curved on logarithmic axes.

Of course there might be other applications I am unaware of where the vector is actually directly related to the X and Y coordinates - did you have such an application in mind?

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Oct 3, 2020

@Jonarw thanks for taking a look. I don't have any particular application in mind (I did 3 or 4 years ago, but I can't remember what; I threw something together at the time and moved on).

I completely understand your point about the magnitudes being meaningful. The vector field with angle/mag only makes sense on a set of well aligned and scaled axes, in which case the representation doesn't matter: a vector is just a vector, and the user could use a FromAngleAndMagnitude method to create them in if suits their use case and enforce a pair of well aligned and scaled axes.

  • With an angle/mag representation, the magnitude is always meaningful, but the angle will be misleading on a pair of mis-aligned/scaled axes.
  • With a dx/dy representation, the direction is always meaningful, but the magnitude will be misleading on a pair of mis-scaled axis

On the other hand, a plot of some arbitrary differential function doesn't necessarily warrant an angle/magnitude interpretation at all. If interpretability is a concern then there should be a constraint on the axes to enforce interpretability, regardless of the representation.

Regarding X/Y being coordinates, this is why I introduce the DataVector type, which is explicitly not a point, but rather a vector. The same has existed for ages with ScreenPoint/ScreenVector.

Regarding curves on log/other axis, I mostly implemented this for fun and to explore alternative to the current algorithm in LineAnnotation. It doesn't make sense with an angle/mag interpretation, but I don't know that an angle/mag interpretation could ever make sense on a pair of non-linear 2D axes.

To sum up, I think that either representation is problematic on mis-aligned/scaled axes, which leads to the conclusion that we should have a more powerful constraints mechanism to enforce consistency between axes. It's very hard to enforce constraints at the moment beyond the Cartesian: we could make this as an extensibility point. In my opinion, x/y is the more natural representation of a vector, but I'm willing to be convinced otherwise.

Could it make sense to have 2 series?

  • one with a data-space vector representation (would it be better to just have a more general purpose stream plot?)
  • one with a angle/mag vector representation

@Jonarw
Copy link
Member

Jonarw commented Oct 3, 2020

Hmm yes I see your point. I'll have to think about it some more.
In the meantime a (possibly stupid) idea: How about the the vector was specified by magnitude and angle, where the magnitude would be specified in screen space (and therefore unaffected by axes) and the angle in data space, so it would still be meaningful when the axes not cartesian. This would in theory solve the problem, but at the moment I am not sure if it wouldn't be too confusing or if makes any sense at all.

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Oct 3, 2020

@Jonarw I might change it so that it is angle/mag, but the mag is data/screen space configurable to see how that feels. I really must take another look at matlab/matplotlib to see what people expect as well.

Note to self: Probably worth opening an issue about extending the plottypes to support more constraints as well.

@cliff-lane
Copy link

Has there been any more thought put into this project? I am in need of a plotting library that includes Contour Plots, Histograms, and Vector Fields. It appears that OxyPlot has everything I need with the exception of the finalized Vector Fields. The data I need to plot is basically misalignment data where I have a (x,y) of where the point should be and a (x,y) of how far off the point is from where it should be.

@VisualMelon
Copy link
Contributor Author

Sorry, no: not worked on this at all.

For your use-case, sounds like it would make sense to have a dx/dy vector description, which is what the draft currently implements. You should be able to mostly C&P the implementation if you want to mess around with it: there's a lot of junk in the PR that shouldn't be there and can be ignored.

@tylerdoboy
Copy link

Will this become available before next year? I have been implementing my own vector field but it would be nice to have this series available.

@VisualMelon
Copy link
Contributor Author

@tylerdoboy seems there is a fair bit of interest in a vector series, so that'll push it higher in my list of things TODO if I have time.

Please do share any details you can about your use-case: would the design here work for you? Are there any features you need that are missing? etc. this will help with the design and let us know when we have something worth including in the library.

@VisualMelon
Copy link
Contributor Author

Rebased, tidied a bit, and added the ability to position vector arrows/text across along their length to support a vector-field scenario.

If anyone has any fun examples they'd like to see implemented to ensure they have good support, let me know.

@VisualMelon
Copy link
Contributor Author

Do we need a nice means for plotting a 2D vector field? (current API demands you provide it with per-vector VectorItems.

This could be a helper that generates a list of points from the 2D array, or it could be an alternative to Items in the series itself, or it could be implemented as a special ItemSource.

@VisualMelon
Copy link
Contributor Author

Definitely need to do some tidying here, and add a more sensible example, but reasonably happy with the API as is. Need to review the data-bounds logic as well.

@VisualMelon
Copy link
Contributor Author

Current data-range is defined by the 'Origin' of each VectorItem; this means that the arrows may be clipped. We could change this so that it's the min/max point where a rendered arrow starts or stops, but I'm not completely sold on that idea.

Copy link
Member

@Jonarw Jonarw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some smaller comments just from looking at the code changes, I will play around with the example later.

Current data-range is defined by the 'Origin' of each VectorItem; this means that the arrows may be clipped. We could change this so that it's the min/max point where a rendered arrow starts or stops, but I'm not completely sold on that idea.

I guess arguments could be made for both, I am not quite sure yet on which side I am on this ;)

Source/OxyPlot/Foundation/DataPoint.cs Outdated Show resolved Hide resolved
Source/OxyPlot/Foundation/DataVector.cs Outdated Show resolved Hide resolved
Source/OxyPlot/Foundation/DataVector.cs Outdated Show resolved Hide resolved
Source/OxyPlot/Foundation/DataVector.cs Show resolved Hide resolved
Source/OxyPlot/Rendering/ScreenPoint.cs Outdated Show resolved Hide resolved
Source/OxyPlot/Rendering/ScreenPoint.cs Outdated Show resolved Hide resolved
Source/OxyPlot/Rendering/ScreenVector.cs Show resolved Hide resolved
Source/OxyPlot/Series/VectorItem.cs Show resolved Hide resolved
@VisualMelon
Copy link
Contributor Author

@Jonarw lots of comments here about DataVector that apply equally to ScreenVector: I'd suggest we handle those in a separate PR where we update both to be less hideous.

@Jonarw
Copy link
Member

Jonarw commented Jul 11, 2023

@Jonarw lots of comments here about DataVector that apply equally to ScreenVector: I'd suggest we handle those in a separate PR where we update both to be less hideous.

Yes, fine with me!

@VisualMelon
Copy link
Contributor Author

VisualMelon commented Jul 11, 2023

Regarding examples, they're pretty terrible at the moment, but I'm happy to implement any more exciting/useful ideas you might have.

@VisualMelon
Copy link
Contributor Author

I've marked the bits that are not vector-series specific as resolved for now to keep things clear for this PR; observations so far:

  • Would prefer everything is immutable
  • If everything is immutable, not point having interval fields for x/y
  • Normalise should use Length rather than computing it independently
  • Use IsNaN instead of x == x everywhere

Copy link
Member

@Jonarw Jonarw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two small, somwhat opinion-based, comments about the API, otherwise I think this is good to merge!

The Examples are sufficient to see that everything works as it should, so they fulfill their purpose I think.

Source/OxyPlot/Series/VectorSeries.cs Outdated Show resolved Hide resolved
Source/OxyPlot/Series/VectorSeries.cs Outdated Show resolved Hide resolved
@VisualMelon VisualMelon marked this pull request as ready for review July 11, 2023 16:04
@VisualMelon
Copy link
Contributor Author

VisualMelon commented Jul 11, 2023

I agree with both of those, and relaxed the protected ActualItems also (should it be IReadOnlyList?): no reason we can't use more sensible types on new series, and if we later decide to change to the concrete type then it's not a code breaking change.

Edit: can't be IReadOnlyList because IList doesn't implement it

@VisualMelon
Copy link
Contributor Author

Forgot the changelog...

@VisualMelon
Copy link
Contributor Author

Hopefully this is in a mergeable state, but I'd rather we didn't merge it just yet, as I'd like to give it another review myself in a couple of days when my head is clearer.

@VisualMelon
Copy link
Contributor Author

Rebased. I don't have any more changes I want to make now.

@VisualMelon
Copy link
Contributor Author

Rebased

@Jonarw Jonarw merged commit 88bfe5f into oxyplot:develop Aug 16, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants