Off-axis (non-SPH) particle projections

[jzuhone]

PR Summary

This PR adds support for off-axis projections of particles which are not SPH particles, by adding this capability to the ParticleProjectionPlot class for plots, and adding a new class FITSParticleOffAxisProjection for FITS files.

Example:

 import yt

 ds = yt.load("IsolatedGalaxy/galaxy0030/galaxy0030")

 L = [1, 1, 1] # normal or "line of sight" vector
 N = [0, 1, 0] # north or "up" vector

 p = yt.ParticleProjectionPlot(ds, L, [("all", "particle_mass")], width=(0.05, 0.05), depth=0.3,
                               north_vector=N)
 p.set_unit(("all", "particle_mass"), "Msun")
 p.save()

Still need to add tests, will do that soon.

PR Checklist

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

[matthewturk]

Looks alright! Minor comments.

[matthewturk]

While I'm thinking of it, I do also think we should consider a mode of rotation for particle-based datasets. Specifically, it seems like we could interpose anywhere the IO happens to have the particles rotated. I don't think this would work with a context manager (necessarily) but maybe could.

[jzuhone]

While I'm thinking of it, I do also think we should consider a mode of rotation for particle-based datasets. Specifically, it seems like we could interpose anywhere the IO happens to have the particles rotated. I don't think this would work with a context manager (necessarily) but maybe could.

This is a good thought--I've recently had occasion to make copies of particle datasets where I have rotated and shifted the coordinates and velocities.

[jzuhone]

I am completely baffled by the Jenkins failures.

[jzuhone]

@yt-fido test this please

[jzuhone]

@Xarthisius any idea what happened with the seemingly unrelated Jenkins failures?

[neutrinoceros]

@jzuhone it's not impossible that those failures are due to a new incompatibility with old unyt pickled data and free ones produced with sympy 1.12 (released 2 days ago).

edit: testing this hypothesis on #4441

[neutrinoceros]

I'll try to make time for a thorough review soon, for now here are a couple comments

[neutrinoceros]

Overall looks great. I got a couple nits and questions, but more importantly, a comment on how to change the name of the argument (axis -> normal) without immediately breaking backward compat.

[neutrinoceros]

finishing with details of the deprecation cycle, I'll try to circle back for an other full review tonight

[neutrinoceros]

Sorry I wasn't able to review as early as I wanted. I have some important concerns with the current design of the ParticleProjectionPlot class, and I'd like it be made closer to ProjectionPlot while we have the chance.

[neutrinoceros]

data_source.normal_vector is not actually used in the following so it is a bit obscure why the branching is done depending on wether or not this attribute exists. If we're going to treat OffAxis...DummyDataSource classes as a protocol, I'd rather we do it more explicitly (yes it's uglier, but also much easier to understand and refactor if we ever manage to)

Suggested change

	if hasattr(data_source, "normal_vector"):
	if re.fullmatch(r"OffAxis\w+DummyDataSource", type(data_source).__name__):

[jzuhone]

I'd prefer we find a less ugly way to do this.

[jzuhone]

It's frankly better to add a comment here why we check this.

[matthewturk]

I think we need to be extremely careful that this doesn't creep out of scope. I would prefer this be a minimally invasive change for new functionality, and the branching -- while perhaps a bit unexpected -- is pragmatic and certainly consistent with other branches.

[neutrinoceros]

There's way to implement this check using isinstance instead of hasattr that

• can be checked continuously
• doesn't require a (possibly outdated) comment
• doesn't require inheritance

Namely: typing.Protocol (which we do have access to since we don't support Python < 3.8 any more)

For illustration, here's a minimal implementation

from typing import Protocol, runtime_checkable
import numpy as np

@runtime_checkable
class OffAxisDummyDataSource:
    normal_vector: np.ndarray

class Dumdumdatasource:
    normal_vector = np.zeros(3)
    
assert isinstance(Dumdumdatasource(), OffAxisDummyDataSource)

I understand that this goes beyond the scope of this PR, which is why I instead suggested an (ugly) one-line refactor to make the code more expressive, but long term I'd like to start using such protocol classes to replace calls to hasattr; IMO they would fit nicely in the yt._typing namespace. Is this something we can all get behind ?

[neutrinoceros]

I'd like an answer to this before I hit approve. I'm happy to put in the work for yt 4.3.0

[neutrinoceros]

@jzuhone

[jzuhone]

I'm obviously happy to discuss anything, and it does look like a better long-term solution, but needless to say we'll have to hash that out in a different forum.

That ok?

[neutrinoceros]

Of course, I just didn't want to leave unresolved threads before merging this important PR.

[jzuhone]

I attempted to implement the new functionality in a way that mimicks what we do for ProjectionPlot, AxisAlignedProjectionPlot, and OffAxisProjectionPlot, but it turns out that this was trickier than it first appeared since there's a lot of shared functionality that's required for these particle plots between the two different cases that is not true for the non-particle plots. There may be a better way to do this but it's way out of scope here.

No idea why the doc build is failing, looks like a separate issue.

[jzuhone]

@yt-fido test this please

[jzuhone]

this is ready to go