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

Allow for different normalizations of magnetic fields in CGS #3812

Merged
merged 5 commits into from
Mar 1, 2022

Conversation

jzuhone
Copy link
Contributor

@jzuhone jzuhone commented Feb 15, 2022

PR Summary

We've been having lots of fun with EM units, so let's have some more!

The relationship between the magnetic field strength and other related quantities, such as magnetic pressure and Alfvén speed, depends on what units are chosen for the magnetic field. For example, the magnetic pressure in CGS is:

image

but in SI/MKSA units it is:

image

We already take care of both of these cases in yt. However, the CGS convention above is known as the "Gaussian" convention, whereas in many MHD simulations and in some physics areas (such as particle physics/GR) it is more common to use the "Lorentz-Heaviside" convention, which is:

image

This was requested in issue #3471 for Athena datasets. This PR addresses that issue by allowing Athena/Athena++ datasets to set the factor for magnetic units in CGS, and then this needs only to be utilized by those fields that need the correct factor to derive other quantities like those mentioned above. In theory this could be implemented for other frontends, but I don't see a pressing need to do so at this time.

Docs forthcoming. I see this as a feature enhancement, so it should wait until 4.1.

PR Checklist

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

@jzuhone jzuhone linked an issue Feb 15, 2022 that may be closed by this pull request
@neutrinoceros neutrinoceros modified the milestone: 4.1.0 Feb 16, 2022
@neutrinoceros
Copy link
Member

I see this as a feature enhancement, so it should wait until 4.1.

Thanks, that answers my first question !
Also for disclosure, what just happened is that I immediately triaged into the 4.1 milestone, then thought that it's preferable to have the issue in the milestone instead, just in case the PR could get stuck or turn out not to be the best solution.

@neutrinoceros neutrinoceros added the code frontends Things related to specific frontends label Feb 16, 2022
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Some thoughts on design and naming:
mag_factor sounds like a scalar, and indeed that's what AthenaDataset.mag_factor is, but the user-exposed parameter should probably use a different name if a str is expected. Otherwise you could expose the scalar directly and keep the names aligned (provided it would be very broken in all but a couple cases).
Now even if we do that, mag_factor is a bit unclear a name, IMHO:

  • no other keyword argument contains shortened words like mag (which could also mean "magnitude" to a clueless user)
  • factor makes it sound like something we multiply B^2 by, when it's really its inverse.

I would also favour making the instance attribute hidden (add a trailing _), and the argument keyword-only, whatever their names.

Also, do you expect that any convention could be used in other frontends that are not already in the 3 you listed and implemented ? if not, could we move the small dict that defines them from AthenaDataset.__init__ to the mag_factors closure, or any other place that's reusable by other frontends ?

@jzuhone
Copy link
Contributor Author

jzuhone commented Feb 17, 2022

Hi @neutrinoceros,

mag_factor sounds like a scalar, and indeed that's what AthenaDataset.mag_factor is, but the user-exposed parameter should probably use a different name if a str is expected. Otherwise you could expose the scalar directly and keep the names aligned (provided it would be very broken in all but a couple cases).

I've changed the name of the argument to magnetic_normalization.

no other keyword argument contains shortened words like mag (which could also mean "magnitude" to a clueless user)

Fair--it's now magnetic_normalization and Dataset._magnetic_factor.

factor makes it sound like something we multiply B^2 by, when it's really its inverse

This is tricky--the "factor" is normally thought of as 4pi (or sqrt(4pi)) because that's the thing that appears in various quantities, whether on the top or the bottom of the fraction.

Also, do you expect that any convention could be used in other frontends that are not already in the 3 you listed and implemented ? if not, could we move the small dict that defines them from AthenaDataset.init to the mag_factors closure, or any other place that's reusable by other frontends ?

I put it in yt.fields.magnetic_fields.

@jzuhone
Copy link
Contributor Author

jzuhone commented Feb 17, 2022

Hi @forrestglines,

Sorry this took so long to address, but here is a fix for the issue you reported.

@@ -486,6 +488,7 @@ def __init__(
self.specified_parameters = parameters.copy()
if units_override is None:
units_override = {}
self._magnetic_factor = cgs_normalizations[magnetic_normalization]
Copy link
Member

Choose a reason for hiding this comment

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

One more idea to make this even better: instead of looking up the value directly from the dictionary, we could wrap it in a small helper function such as

def get_magnetic_normalization(key:str) -> float:
    if not key in cgs_normalizations:
        raise ValueError(
            "Unknown magnetic normalization convention. "
            f"Got {key!r}, expected one of {tuple(cgs_normalizations)}
        )
    return cgs_normalizations[key]

so that valid values are visible in the error message (instead of the raw KeyError)

@neutrinoceros
Copy link
Member

Thanks John for addressing my comments so far, I think it's already in a much better state now !

@neutrinoceros neutrinoceros merged commit e2ec7e2 into yt-project:main Mar 1, 2022
@jzuhone jzuhone deleted the b_normalization branch June 21, 2022 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code frontends Things related to specific frontends enhancement Making something better units
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lorentz-Heaviside Magnetic Units vs CGS
3 participants