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

Support triangular plots #98

Merged
merged 2 commits into from Jul 11, 2017

Conversation

Projects
None yet
2 participants
@bocklund
Collaborator

bocklund commented Jun 7, 2017

Major points:

  • Add triangular matplotlib projection
  • Add ternplot API that functions similarly to binplot
  • Both ternplot and binplot use eqplot. The behavior of eqplot is controlled by the projection. When 3D plotting etc. is added, the constraints for different projections can simply be modified

Minor points:

  • ternplot uses similar API and semantics as binplot
  • Fix binplot error when only single phase regions exist (e.g. only liquid, even though it's not an interesting diagram)
  • Add _axis_label function to more generally handle axis labels
  • Small docs fixes

Pay attention to:

  • Is there a better way to transform the y axis label in the triangular plot? (eqplot line 98)
  • Any ideas on eqplot line 124? I think we could do that and it would make things a little cleaner. Especially because we could just append all of the found phase stuff together.
  • Anything seem wrong in my ternary colors? If you make the symbol sizes larger, you can see that the three phase ones are miscolored
  • Should we sort the independent component list in the ternplot API to get more consistent (alphabetic) plotting or just let it be unless the user specifies with x= and y= in ternplot?

Example code:

from matplotlib import pyplot as plt
from pycalphad import Database, ternplot, variables as v
dbf = Database(database_file)
phases = list(dbf.phases.keys())
comps = ['AL','CU','ZR','VA']
fig = plt.figure(figsize=(20,20))
conditions = {v.X('ZR'): (0,1,0.01), v.X('AL'): (0,1,0.01), v.P: 101325, v.T: 1000}
ternplot(dbf, comps, phases, conditions, x=v.X('ZR'), y=v.X('AL')) 
@bocklund

This comment has been minimized.

Collaborator

bocklund commented Jun 16, 2017

@richardotis This was updated comments made offline:

  • Removed projection kwarg from eqplot. Plot type (projection) is determined by the degrees of freedom in the passed equilibrium.
    This might make isopleths more confusing to call with the eqplot API, but also more explicit by passing the eq object after the correct number of DOF have been selected with xarray.

  • Remove the phases argument from eqplot. The phases argument doesn't make sense because you wouldn't ever want to plot a phase that may be in stable equilibrium in eqplot.

@richardotis

This comment has been minimized.

Collaborator

richardotis commented Jun 16, 2017

FYI regarding one of the failing tests: numpy/numpy#9251

@richardotis richardotis added this to the 0.6 milestone Jun 23, 2017

@richardotis

I mentioned this in one comment, but the final version of the PR should have at least one test.

elif len(indep_comps) == 2 and len(indep_pots) == 0:
projection = 'triangular'
else:
raise ValueError('The eq_plot projection is not defined and cannot be autodetected. There are {} independent compositions and {} indepedent potentials.'.format(len(indep_comps), len(indep_pots)))

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

eqplot not eq_plot

# TODO: Temporary workaround to fix string encoding issue when loading netcdf files from disk
phases = phases if phases is not None else \
map(str, sorted(set(np.array(eq.Phase.values.ravel(), dtype='U')) - {''}, key=str))
ax = plt.gca(projection=projection) if ax is None else ax

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

I suspect if you call eqplot multiple times in a notebook cell with this code, it will overplot on top of the same figure.
Something like this may be preferable:

if ax is None:
    fig = plt.figure(figsize=(6,6)) # or whatever size
    ax = fig.gca(projection=projection)
x = indep_comps[0] if x is None else x
y = indep_pots[0] if y is None else y
# plot settings
ax.set_xlim([np.min(conds[x]) - 1e-2, np.max(conds[x]) + 1e-2])

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

Do users need the ability to turn these axes adjustments off? I'm struggling to think of an instance where I may want to overplot multiple calls of eqplot on the same figure, but wouldn't just adjust the axes to my liking after the calls to eqplot.

This comment has been minimized.

@bocklund

bocklund Jun 23, 2017

Collaborator

eqplot returns the Axes object so if someone wanted to customize anything I think they should just do it in matplotlib directly. IMO we should choose a few sane defaults for the common path and allow users to do fine adjustments in matplotlib or an image editor.

# We make it reasonably comparable to the position of the xlabel from the xaxis
# As the figure size gets very large, the label approaches ~0.55 on the yaxis
# 0.55*cos(60 deg)=0.275, so that is the xcoord we are approaching.
# TODO: it would be nice to handle this automatically

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

I have always ended up having to manually tweak axis labels, so hard-coding magic numbers is probably the best option for the near term. Users can always pass in their own ax and tweak it after the fact.

legend_handles, colorlist = phase_legend(phases)
# position the phase legend
# TODO: can we concatenate the last dimension of these axes to have just one plot command for tielines and another for points?

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

This probably isn't important.

ax.yaxis.set_label_coords(x=(0.275 - y_label_offset), y=0.5)
# get the active phases and support loading netcdf files from disk
phases = map(str, sorted(set(np.array(eq.Phase.values.ravel(), dtype='U')) - {''}, key=str))

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

There should probably be a test which verifies that eqplot(eq) == eqplot(deserialize(serialize(eq)). This could probably be done by writing the output of the LHS and RHS to files which should compare bitwise identical

# For both two and three phase, cast the tuple of indices to an array and flatten
# If we found two phase regions:
if len(np.array(two_phase_idx).flatten()) > 0:

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

You can simplify this with two_phase_idx.size > 0

ax.add_collection(lc)
# If we found three phase regions:
if len(np.array(three_phase_idx).flatten()) > 0:

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

You can simplify this with three_phase_idx.size > 0

three_phase_tielines = np.rollaxis(three_phase_tielines,1)
three_lc = mc.LineCollection(three_phase_tielines, zorder=1, colors=[1,0,0,1], linewidths=[0.5, 0.5])
# plot three phase points and tielines
# TODO: the colors seem to be ordered wrong on the plots

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

What does this mean?

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

The TODO comment, I mean

This comment has been minimized.

@bocklund

bocklund Jun 23, 2017

Collaborator

The coloring of the markers seemed wrong on the tie triangles I was looking at. The three marker colors were usually correct, but they were on the wrong vertex of the triangle.

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

I think you are selecting the first, second and third rows when you should be selecting the columns.

None yet.
"""
eq_kwargs = eq_kwargs if eq_kwargs is not None else dict()
indep_comps = [key for key, value in conds.items() if isinstance(key, v.Composition) and len(np.atleast_1d(value)) > 1]

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

I recommend sorted(conds.items()) so that the plot is deterministic

lc = mc.LineCollection(tielines, zorder=1, colors=[0,1,0,1], linewidths=[0.5, 0.5])
# plot two phase points and tielines
two_phase_plotcolors = np.array(list(map(lambda x: [colorlist[x[0]], colorlist[x[1]]], found_two_phase)), dtype='U') # from pycalphad
ax.scatter(two_phase_x[..., 0], two_phase_y[..., 0], s=3, c=two_phase_plotcolors[:,0], edgecolors='None', zorder=2, **kwargs)

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

Note the use of two_phase_plotcolors[:,0] -- selected by column

# plot three phase points and tielines
# TODO: the colors seem to be ordered wrong on the plots
three_phase_plotcolors = np.array(list(map(lambda x: [colorlist[x[0]], colorlist[x[1]], colorlist[x[2]]], found_three_phase)), dtype='U') # from pycalphad
ax.scatter(three_phase_x[..., 0], three_phase_y[..., 0], s=3, c=three_phase_plotcolors[0], edgecolors='None', zorder=2, **kwargs)

This comment has been minimized.

@richardotis

richardotis Jun 23, 2017

Collaborator

It should be three_phase_plotcolors[:,0] and so on

@bocklund

This comment has been minimized.

Collaborator

bocklund commented Jun 25, 2017

I addressed everything except for the test. I think the proposed test would be hard to maintain.

If there are some specific things you want to test, I can break those out into functions and test them. Or possibly just do an integration test to see that eqplot returns an Axes object.

@richardotis

This comment has been minimized.

Collaborator

richardotis commented Jun 25, 2017

An integration test would be fine. What needs to happen to fix the tests on AppVeyor?

@bocklund

This comment has been minimized.

Collaborator

bocklund commented Jun 25, 2017

Looks like the NumPy issue from above and a failure in a warning test because we are also getting a warning for using dask.async.get_sync which is deprecated in favor of dask.local.get_sync

@bocklund

This comment has been minimized.

Collaborator

bocklund commented Jul 10, 2017

@richardotis this is ready for another look once tests pass

@bocklund bocklund force-pushed the triangular branch from cdc8c4f to 31cede4 Jul 10, 2017

@bocklund

This comment has been minimized.

Collaborator

bocklund commented Jul 10, 2017

@richardotis looks like the AppVeyor build of on py34 that uses sympy<1.1 (#105) is passing.

This is ready to be squashed and merged with review approval.

bocklund added some commits Jun 5, 2017

ENH: Add ternary plotting and eqplot
General changes:
* Add helper _axis_label function to eqplot module.
* Fixes the plotting for binary plots with only single phase regions
* Remove phases argument from plotting functions. We should always plot
  all of the phases we calculated in equilibrium.
* Determine the type of plotting, (for now binary/ternary) by the number
  of independent components and potentials in the equilibrium xarray.

Ternary additions:
* Add ternplot module/function that uses the same API and semantics as
  binplot. Added to main namespace.
* Add triangular projection module.
* Allow specification of x and y axes in ternplot. If unspecified, x and
  y will be ordered by sorted compositon names.

@bocklund bocklund force-pushed the triangular branch from 31cede4 to addf5cc Jul 10, 2017

@richardotis richardotis merged commit b6803d6 into develop Jul 11, 2017

3 of 5 checks passed

continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.4%) to 86.599%
Details

@bocklund bocklund deleted the triangular branch Oct 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment