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

NCL_axes_3.py #313

Merged
merged 9 commits into from
Mar 26, 2021
Merged

NCL_axes_3.py #313

merged 9 commits into from
Mar 26, 2021

Conversation

hCraker
Copy link
Contributor

@hCraker hCraker commented Feb 25, 2021

Closes #310
image

@hCraker hCraker added the work in progress Not ready for review yet label Feb 25, 2021
@hCraker
Copy link
Contributor Author

hCraker commented Feb 26, 2021

image

@hCraker hCraker added ready for review Development is done and ready for reviews and removed work in progress Not ready for review yet labels Feb 26, 2021
@hCraker
Copy link
Contributor Author

hCraker commented Feb 26, 2021

It is an extremely small detail, but I noticed that gvutil.add_major_minor_ticks sets the value of the minor tick mark to be 5.5e2 for the top right plot and 5.5e1 and 5.5e2 for the lower left one. It's not an issue for this example, but it is an error in that function since the value should be 5e2. I will be opening a PR in geocat-viz to fix it.

Plots/XY/NCL_axes_3.py Outdated Show resolved Hide resolved
Co-authored-by: Michaela Sizemore <43652875+michaelavs@users.noreply.github.com>
@hCraker hCraker marked this pull request as ready for review March 1, 2021 17:41
@hCraker hCraker requested a review from michaelavs March 1, 2021 17:41
Copy link
Member

@anissa111 anissa111 left a comment

Choose a reason for hiding this comment

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

This looks good, but I noticed you're using a different comment style than previous examples, like on line 41:

##### Create plot with linear axes and full perimeter #####.

Is there a reason that you've formatted them like this?

@hCraker
Copy link
Contributor Author

hCraker commented Mar 3, 2021

This looks good, but I noticed you're using a different comment style than previous examples, like on line 41:

##### Create plot with linear axes and full perimeter #####.

Is there a reason that you've formatted them like this?

@anissa111 I did this to make it very clear when looking at the code when I'm making one subplot versus another. Let me know if you have a better way of making this distinction. It's purely for readability.

@hCraker hCraker requested a review from anissa111 March 8, 2021 15:44
Copy link
Member

@anissa111 anissa111 left a comment

Choose a reason for hiding this comment

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

I understand why you've included the different comment style, but I think it should be altered to closer match the style of existing comments.

Do you think it would be okay to do something like
# Subplot (1, 1): Create plot with linear axes and full perimeter
would work?

I looked into modifying how the Sphinx RTD image scrapers work to allow for sectioning with the markdown sectioning, but it doesn't look very promising.

@hCraker hCraker requested a review from anissa111 March 18, 2021 14:36
Copy link
Member

@anissa111 anissa111 left a comment

Choose a reason for hiding this comment

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

Thank you for making that change!

@anissa111 anissa111 merged commit b57eeab into main Mar 26, 2021
@anissa111 anissa111 deleted the axes_3 branch March 26, 2021 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Development is done and ready for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

axes_3
3 participants