Skip to content

Conversation

@NicolasPietteLauziere
Copy link
Contributor

@NicolasPietteLauziere NicolasPietteLauziere commented Jun 17, 2021

Added AIOCG plotting capabilities

Description

Added AIOCG scatterplots capabilities

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Added AIOCG plotting capabilities
@morganjwilliams morganjwilliams self-requested a review June 21, 2021 03:58
@morganjwilliams morganjwilliams added the enhancement New feature or request label Jun 21, 2021
@morganjwilliams morganjwilliams added this to the 0.3.1 milestone Jun 21, 2021
Copy link
Owner

@morganjwilliams morganjwilliams left a comment

Choose a reason for hiding this comment

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

Hey @NicolasPietteLauziere!

I've added a range of comments here, some around changing a few names here and there, but largely around the 600-400 plot coordinates; I think this relates to pixels rather than the data coordinates? If so I think try to keep everything in the natural molar data coordinates and add a ax.set_aspect(4/6) within the add_to_axes(...) function - this should achieve the same effect.

Could you also add a short import in pyrolite.plot.templates.__init__.py along the lines of the following:

from .AIOCG import AIOCG

After a few changes here and there this should be ready to merge. If you get stuck with any of the suggestions or have any questions feel free to ping me! If needed I can also make changes after merging into develop to round it off.

if cfg["poly"]:
verts = np.array(cfg["poly"]) * rescale_by
x, y = get_centroid(matplotlib.patches.Polygon(verts))
label = cfg["name"][0]
Copy link
Owner

Choose a reason for hiding this comment

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

For things which don't have names, you can leave the annotation step out:

if label:
    ax.annotate(
        ...
    )

Whether to fill the polygons.
axes_scale : :class:`float`
Maximum scale for the axes. Typically 100 (for wt%) or 1 (fractional).
labels : :class:`str`
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this classifier has different sets of labels, so if you do accept the labels kwarg, there should be a logger.info(...) or that it doesn't do anything, and generally I'd remove it from the docstring.

Axis to add the polygons to.
fill : :class:`bool`
Whether to fill the polygons.
axes_scale : :class:`float`
Copy link
Owner

Choose a reason for hiding this comment

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

I think the default scale for this is 1.0; and then you'll want to set the aspect of the Axes to 6:4?

@@ -0,0 +1 @@
{"name":"AIOCG","axes":{"x":"(2Ca+5Fe+2Mn)+(2Ca+5Fe+2Mn+Mg+Si) molar","y":"K/(K+Na+0.5Ca) molar"},"fields":{"K":{"name":["K"],"poly":[[0,405.2],[103.2,405.2],[109.4,244.1],[51.6,235.0],[39.6,222.8],[45.6,194.4],[0,194.4]]},"KintNa":{"name":[""],"poly":[[0,194.4],[0,121.5],[145.7,121.5],[54.7,176.2],[45.6,194.4]]},"Na":{"name":["Na"],"poly":[[0,121.5],[145.7,121.5],[145.7,32.4],[157.9,0.0],[0,0.0]]},"NotAltered1":{"name":[""],"poly":[[291.6,145.9],[221.7,72.5],[294.7,30.5],[346.3,44.6],[352.3,54.7],[346.3,73.0],[334.1,101.3],[315.8,117.6]]},"NotAltered2":{"name":[""],"poly":[[291.6,145.9],[221.7,72.5],[294.7,30.5],[346.3,44.6],[352.3,54.7],[346.3,73.0],[334.1,101.3],[315.8,117.6]]},"Na-Ca-Fe-(Mg)":{"name":["Na-Ca-Fe-(Mg)"],"poly":[[145.7,121.5],[145.7,32.4],[157.9,0.0],[413,0.0],[400.8,48.7],[367.4,77.1],[352.3,117.6],[315.8,117.6],[334.1,101.3],[346.3,73.0],[352.3,54.7],[346.3,44.6],[294.7,30.5],[206.6,81.1]]},"KintK-Fe":{"name":[""],"poly":[[103.2,405.2],[109.4,244.1],[115.4,245.1],[197.5,223.5],[200.4,275.6],[194.4,405.2]]},"K-Fe":{"name":["K-Fe"],"poly":[[194.4,405.2],[200.4,275.6],[197.5,223.5],[200.4,222.8],[437.2,222.8],[431.2,328.1],[413,364.6],[413,405.2]]},"Ca-K-Fe":{"name":["Ca-K-Fe"],"poly":[[200.4,222.8],[291.6,145.9],[315.8,117.6],[352.3,117.6],[443.5,117.6],[437.2,222.8]]},"Ca-Fe-(Mg)":{"name":["Ca-Fe-(Mg)"],"poly":[[352.3,117.6],[367.4,77.1],[400.8,48.7],[413,0.0],[461.7,0.0],[445.4,70.8],[443.5,117.6]]},"Fe-rich_Ca-Fe":{"name":["Fe-rich Ca-Fe"],"poly":[[443.5,117.6],[445.4,70.8],[461.7,0.0],[607.4,0.0],[607.4,117.6]]},"Fe-rich_Ca-K-Fe":{"name":["Fe-rich Ca-K-Fe"],"poly":[[437.2,222.8],[443.5,117.6],[607.4,117.6],[607.4,222.8]]},"Fe-rich_K-Fe":{"name":["Fe-rich K-Fe"],"poly":[[413,405.2],[413,364.6],[431.2,328.1],[437.2,222.8],[607.4,222.8],[607.4,405.2]]}}}
Copy link
Owner

Choose a reason for hiding this comment

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

A quick question about these coordinates - are they in pixels? It would be good to convert them back to molar proportions if so, and then just set the aspect of the axes on which the diagram is plotted to the 6-4 ratio which it's typically found in.


with open(src, "r") as f:
config = json.load(f)
kw = dict(scale=100.0, xlim=[0, 607.4], ylim=[0, 405.2])
Copy link
Owner

Choose a reason for hiding this comment

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

I think the scale should be 1.0 here; and if data coordinates (0-1) are separated from the plot aspect (6:4), the limits should both be (0, 1).

# )


ax.set_ylabel("$K/(K+Na+0.5Ca) molar$")
Copy link
Owner

Choose a reason for hiding this comment

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

I think you can probably leave out the molar in the plot axes labels, or at least move it outside the $mathtext$ section so it's rendered as standard text.

@morganjwilliams
Copy link
Owner

Hey @NicolasPietteLauziere, just giving this one a bump. Let me know if you're happy to (and have time) to make these changes to your AIOCG plot before I merge it in. Cheers!

@NicolasPietteLauziere
Copy link
Contributor Author

Hi Morgan, I'm happy to do the changes. It will take me a bit of time to do it as I am finishing my thesis final chapter (!!!).

Edits following Owner's comments. Not stable yet.
@morganjwilliams
Copy link
Owner

@NicolasPietteLauziere, no worries! Good luck with the writing up in the meantime! 😄

I'll update the target release for this one to v0.3.2.

@morganjwilliams morganjwilliams modified the milestones: 0.3.1, 0.3.2 Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants