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

AIOCG plots #57

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

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.

@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.

pyrolite/geochem/alteration.py Show resolved Hide resolved
pyrolite/plot/templates/AIOCG.py Outdated Show resolved Hide resolved
pyrolite/plot/templates/AIOCG.py Outdated Show resolved Hide resolved
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(
        ...
    )

pyrolite/util/classification.py Outdated Show resolved Hide resolved
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