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

ENH: ProjectionPlot as a factory function #3450

Closed

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Jul 22, 2021

PR Summary

Following a question by @chummels on Slack, I'm trying here to make yt.ProjectionPlot work similarly to SlicePlot, i.e, as a factory function rather than a class, so as to embrace OffAxisProjectionPlot as well as AxisAlignedProjectionPlot, which is here the new name of the old ProjectionPlot class.

Two remarks:

  • the user-facing API in SlicePlot gets much simpler to maintain and reproduce after the end of the 4.0 -> 4.1 deprecation cycle, so I applied the same patch as in DEPR: deprecation cycle (4.0 -> 4.1) #3240 where due.
  • since I needed to reuse some crucial input-sanitizing code from SlicePlot regarding the normal argument, I refactored it a little to improve the specificity of error messages

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@neutrinoceros neutrinoceros added api-consistency naming conventions, code deduplication, informative error messages, code smells... new feature Something fun and new! viz: 2D labels Jul 22, 2021
@neutrinoceros neutrinoceros force-pushed the projection_plot_factory branch 10 times, most recently from 6fbdb5c to 50803ec Compare July 22, 2021 20:02
@neutrinoceros
Copy link
Member Author

A potentially controversial implementation detail is how I approached consistency in the existing API, trying to improve it without breaking backwards compatibility.
Here's a summary of the proposed (opionated) changes as of the current state of this branch:

I'm introducing snake_case names for factory functions (SlicePlot is now defined as slice_plot) and reserve CamelCase for classes, as is custom in Python.
I however preserve backwards compatibility by aliasing SlicePlot (function) to slice_plot.

ProjectionPlot is slightly more delicate, because it's currently a class on the main branch, but it needs to become a function to be made consistent with SlicePlot. This is done here, and constitutes a perfectly transparent change with the exception that it would break any downstream code that relies on the fact that it's a class/type. I'll take a wild guess and assume that these cases are extremely rare if they even exist. To me this feels ok as long as it's properly mentioned in the release notes.

In summary here's the change I have now:

  • renamed: ProjectionPlot (class) becomes AxisAlignedProjectionPlot (class)
  • a new factory function projection_plot, aliased to ProjectionPlot to preserve backwards compatibility and to offer a consistent API with SlicePlot.

Short term, I don't think there is any need to deprecate camel-cased ProjectionPlot and SlicePlot factory functions as it would create a massive amount of warnings downstream for an extremely marginal benefit on our side. However I believe it could be a good idea to start encouraging the proposed new API using snake-cased function names in our documentation and codebase, which can be done with a clean sweeping change.

Long term, maybe we'll want to define ProjectionPlot and SlicePlot as abstract types at some point in the future when typing is much more the norm in the Python ecosystem (likely not before Python 3.9 or 3.10 becomes our oldest supported Python version so I would bet not earlier than 2024), and then formal deprecation could be introduced, and feel much less disruptive if yt's doc has been using snake-case function for several years at that point.

I'll leave this open for discussion and reserve the sweeping change for a follow up PR if it is deemed worth by other maintainers.

@neutrinoceros neutrinoceros force-pushed the projection_plot_factory branch 3 times, most recently from cfaaed2 to 7bd9fc2 Compare July 23, 2021 13:05
@neutrinoceros neutrinoceros marked this pull request as ready for review July 23, 2021 14:41
@neutrinoceros
Copy link
Member Author

I'm opening this for review. I'd like to keep documentation out of this PR so as to make it as easy to review as possible. Any feedback is appreciated.

@neutrinoceros
Copy link
Member Author

pinging @matthewturk @chummels and @brittonsmith for reviews/feedback since you guys participated in the discussion on Slack.

@jzuhone
Copy link
Contributor

jzuhone commented Jul 23, 2021

So changing SlicePlot and ProjectionPlot to slice_plot and projection_plot, while well-motivated by Python custom, is a pretty substantial change from the perspective of yt. I'm not opposed to this change, but I feel pretty strongly that in belongs in a major version release (even with the mitigating factor of aliases).

@munkm
Copy link
Member

munkm commented Jul 23, 2021

Is documentation challenging to review? I'm not sure I understand.

@neutrinoceros
Copy link
Member Author

I'm not opposed to this change, but I feel pretty strongly that it belongs in a major version release (even with the mitigating factor of aliases).

@jzuhone Would it be fine to introduce the snake_case names as the aliases instead, even undocumented ? Or would you prefer we avoid the double api altogether and reserve the transition for an hypothetical version 5.0.0 ?

@munkm two reasons:

  • I would like to avoid iterating too much on docs which is why I'm requesting early feedback
  • say I was going to include a sweeping change in this PR like find-and-replace SlicePlot -> slice_plot over the whole code base, then the important changes would be drown into a gigantic diff, so I prefer collecting you guys' hot takes on it before doing something like this

@jzuhone
Copy link
Contributor

jzuhone commented Jul 24, 2021

Would it be fine to introduce the snake_case names as the aliases instead, even undocumented ?

Yeah, that sounds good to me.

@neutrinoceros neutrinoceros force-pushed the projection_plot_factory branch 2 times, most recently from e6ff406 to ce567af Compare July 24, 2021 12:13
@neutrinoceros
Copy link
Member Author

@jzuhone I've updated the PR to integrate this, and provided a rather extensive historical explanation as a comment.

@neutrinoceros neutrinoceros force-pushed the projection_plot_factory branch 6 times, most recently from f441515 to 6bad0f8 Compare July 24, 2021 15:53
yt/visualization/plot_window.py Outdated Show resolved Hide resolved
# e.g., rounding errors, parsing error...
assert_raises(TypeError, _sanitize_normal_vector, ds, ui)
for ui in [
"X",
Copy link
Member

Choose a reason for hiding this comment

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

Should be reject X, Y and Z?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO yes. If we start making part of the public API case-insensitive for no reason it may create friction later and it will create inconsistencies now.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough

if retv.shape != (3,):
raise ValueError(f"{normal} is incorrectly shape.")
except ValueError as exc:
raise TypeError(f"{normal} is not a valid normal vector.") from exc
Copy link
Member

Choose a reason for hiding this comment

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

Is it really a TypeError?

Copy link
Member Author

Choose a reason for hiding this comment

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

Arguably. Quoting the standard lib: (https://docs.python.org/3/library/exceptions.html#TypeError):

Passing arguments of the wrong type (e.g. passing a list when an int is expected) should result in a TypeError, but passing arguments with the wrong value (e.g. a number outside expected boundaries) should result in a ValueError.

So here, numpy is raising ValueError when it fails to cast values to floats, but really it seems to me that it's a TypeError by definition

Copy link
Member

@cphyc cphyc Jul 27, 2021

Choose a reason for hiding this comment

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

Either way is fine to me 🤷

yt/visualization/plot_window.py Outdated Show resolved Hide resolved
…Plot, introducing the AxisAlignedProjectionPlot class. Add snake case aliases projection_plot -> ProjectionPlot and slice_plot -> SlicePlot, and update SlicePlot's docstring
@neutrinoceros neutrinoceros force-pushed the projection_plot_factory branch from c8a78a7 to 3e4ef92 Compare July 27, 2021 16:11
@neutrinoceros neutrinoceros changed the title FEAT: ProjectionPlot as a factory function ENh: ProjectionPlot as a factory function Aug 29, 2021
@neutrinoceros neutrinoceros changed the title ENh: ProjectionPlot as a factory function ENhH: ProjectionPlot as a factory function Aug 29, 2021
@neutrinoceros neutrinoceros changed the title ENhH: ProjectionPlot as a factory function ENH: ProjectionPlot as a factory function Aug 29, 2021
@neutrinoceros
Copy link
Member Author

I want to rework this in a way that ProjectionPlot and SlicePlot can be classes instead of factory functions. Switching to draft until I either figure it out or give up.

@neutrinoceros
Copy link
Member Author

This is now superseded and replaced by #3489

@neutrinoceros neutrinoceros deleted the projection_plot_factory branch August 29, 2021 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-consistency naming conventions, code deduplication, informative error messages, code smells... new feature Something fun and new! viz: 2D
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants