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

Subdomain extraction #86

Merged
merged 33 commits into from
Jun 30, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
41c1bae
added matmul
Adnan-Ali-Ahmad Feb 18, 2022
bf66a73
spatial PR commit
Adnan-Ali-Ahmad Feb 18, 2022
7fe084a
array minor change
Adnan-Ali-Ahmad Mar 15, 2022
2a8618d
minor stuff
Adnan-Ali-Ahmad Mar 28, 2022
86c9e21
temp stash
Adnan-Ali-Ahmad Apr 25, 2022
5a5acb0
beginning merge
Adnan-Ali-Ahmad May 31, 2022
f959133
Merge remote-tracking branch 'upstream/main' into dev_spatial
Adnan-Ali-Ahmad May 31, 2022
28dff7f
subdomain extraction & coord transforms
Adnan-Ali-Ahmad Jun 2, 2022
fe892fc
spatial package added
Adnan-Ali-Ahmad Jun 2, 2022
e179dcf
merged with dev
Adnan-Ali-Ahmad Jun 2, 2022
9711bea
minor yapf
Adnan-Ali-Ahmad Jun 2, 2022
7ca0cbc
forgot import of subdomain
Adnan-Ali-Ahmad Jun 2, 2022
a0071b3
small changes
Adnan-Ali-Ahmad Jun 7, 2022
8a26e2b
cylindrical transforms
Adnan-Ali-Ahmad Jun 7, 2022
205adf4
Merge branch 'main' into spatial2
Adnan-Ali-Ahmad Jun 13, 2022
a4800cb
slicing PR. This is subdomain extraction
Adnan-Ali-Ahmad Jun 13, 2022
509713c
Merge branch 'spatial2' of https://github.com/Adnan-Ali-Ahmad/osyris …
Adnan-Ali-Ahmad Jun 13, 2022
e2018f0
add new line
Adnan-Ali-Ahmad Jun 13, 2022
3af9990
revert name assignement change
Adnan-Ali-Ahmad Jun 13, 2022
015d006
revert spherical component computations for now
Adnan-Ali-Ahmad Jun 13, 2022
8faa3ff
newline
Adnan-Ali-Ahmad Jun 13, 2022
81ef3e5
Update src/osyris/spatial/__init__.py
Adnan-Ali-Ahmad Jun 14, 2022
4be4096
Update src/osyris/spatial/spatial.py
Adnan-Ali-Ahmad Jun 14, 2022
a88c647
Update src/osyris/spatial/spatial.py
Adnan-Ali-Ahmad Jun 14, 2022
2d6b805
renaming spatial.py to subdomain.py
Adnan-Ali-Ahmad Jun 14, 2022
b5c3d6e
reverted name setter in .to method
Adnan-Ali-Ahmad Jun 14, 2022
668789a
removing parent and meta attribution
Adnan-Ali-Ahmad Jun 15, 2022
cd909e7
removing loader from subdomain
Adnan-Ali-Ahmad Jun 24, 2022
6fd8903
implemented shape check
Adnan-Ali-Ahmad Jun 27, 2022
98b8702
import dataset
Adnan-Ali-Ahmad Jun 27, 2022
16e522a
warning message added
Adnan-Ali-Ahmad Jun 30, 2022
7896bc5
merging with usptream
Adnan-Ali-Ahmad Jun 30, 2022
398962c
flake8
Adnan-Ali-Ahmad Jun 30, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/osyris/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
from .config import config, units
from .plot import histogram1d, histogram2d, plane, scatter, map, plot
from .core import Array, Datagroup, Dataset, Plot
from .spatial import spatial
23 changes: 22 additions & 1 deletion src/osyris/core/array.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ def __getitem__(self, slice_):
unit=self._unit,
parent=self._parent,
name=self._name)
if isinstance(slice_, int) and self.ndim > 1:
if self.shape[0] == 1:
return out
if (isinstance(slice_, int)
or isinstance(slice_, np.integer)) and self.ndim > 1:
return out.reshape(1, len(out))
return out

Expand Down Expand Up @@ -253,6 +256,24 @@ def __mul__(self, other):
unit=1.0 * result.units)
return self.__class__(values=self._array * other, unit=self._unit)

def __matmul__(self, other):
if isinstance(other, self.__class__):
scale_l = self._unit.to_base_units()
scale_r = other._unit.to_base_units()
result = scale_l * scale_r
lhs = self._array
rhs = other._array * result.magnitude
lhs, rhs = self._broadcast(lhs, rhs)
return self.__class__(values=np.transpose(lhs @ rhs),
unit=1.0 * result.units)
if isinstance(other, Quantity):
scale_l = self._unit.to_base_units()
scale_r = other.to_base_units()
result = scale_l * scale_r
return self.__class__(values=self._array * result.magnitude,
unit=1.0 * result.units)
return self.__class__(values=self._array @ other, unit=self._unit)

def __imul__(self, other):
if isinstance(other, self.__class__):
scale_l = self._unit.to_base_units()
Expand Down
6 changes: 6 additions & 0 deletions src/osyris/spatial/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# SPDX-License-Identifier: BSD-3-Clause
# Copyright (c) 2022 Osyris contributors (https://github.com/nvaytet/osyris)
Adnan-Ali-Ahmad marked this conversation as resolved.
Show resolved Hide resolved

# flake8: noqa

from .spatial import cartesian_to_spherical, spherical_to_cartesian, rotation_matrix
89 changes: 89 additions & 0 deletions src/osyris/spatial/spatial.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
# SPDX-License-Identifier: BSD-3-Clause
# Copyright (c) 2022 Osyris contributors (https://github.com/nvaytet/osyris)
Adnan-Ali-Ahmad marked this conversation as resolved.
Show resolved Hide resolved

import numpy as np
from ..core import Array
from ..config import units


def rotation_matrix(vec, angle):
"""
Returns 3D rotation matrix of angle 'angle' around rotation vector 'vec'.
"""
if isinstance(vec, list):
vec = np.array(vec)
vec = vec / np.linalg.norm(vec)
R = np.cos(angle) * np.identity(3) + (np.sin(angle)) * np.cross(
vec,
np.identity(vec.shape[0]) * -1) + (1 - np.cos(angle)) * (np.outer(vec, vec))
return R


def spherical_to_cartesian(r, theta, phi):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I appreciate you wanting to make a start on this earlier rather than later, but as I've said before I think it's worth taking our time to think properly about how we want to implement this.

We have to ask ourselves what is the goal here.
If we want to be able to e.g. easily compute a radius, and make a radial profile, then these functions would definitely be useful.

If we wish to make maps that depend on theta and phi (which was your original motivation), we also need to consider how this would then be used to make the maps. Maybe this approach does work, I am not sure, I have not thought enough about it.

Another approach could have been to give the Array additional properties, such as .r, .theta, and .phi, which would compute the radius and angles on-the-fly. I am thinking that the coordinates are inherently cartesian, because they are as such inside Ramses. So they should be loaded as such, and am therefore not sure that converting from spherical back to cartesian would be something that is ever needed?

In addition, as you pointed out when you first suggested this idea, velocity or B field vectors could also have radial, theta and phi components (and those calculations would be a little different from that of the positions), which would certainly be useful.

This last point, combined with the slicing inconsistency of vectors in the other PR, is making me wonder whether we want a slightly different class for vector arrays (that would inherit from Array).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another approach could have been to give the Array additional properties, such as .r, .theta, and .phi, which would compute the radius and angles on-the-fly.

In addition, as you pointed out when you first suggested this idea, velocity or B field vectors could also have radial, theta and phi components (and those calculations would be a little different from that of the positions), which would certainly be useful.

Maybe we can group these into .get_spherical_components() & .get_cylindrical_components() methods? These shouldn't be automatically computed by osyris upon loading the dataset because otherwise it comes with an extra RAM burden, but they can provide a quick and easy way to gather all components.

This last point, combined with the slicing inconsistency of vectors in the other PR, is making me wonder whether we want a slightly different class for vector arrays (that would inherit from Array).

Instead of having a different class for vectors, I would rather have the type be an attribute of the Array class (eg. a .vector boolean attribute).

"""
Converts spherical components radius, colatitude and azimuth to x,y,z
Returns an osyris array of shape (len(r), 3) containing a stacked xyz
"""
x, y, z = (r * np.cos(phi) * np.sin(theta), r * np.sin(phi) * np.sin(theta),
r * np.cos(theta))
xyz = np.vstack([x, y, z])
return Array(values=np.transpose(xyz).values, unit=r.unit, name="position")


def cartesian_to_spherical(position, origin=[0, 0, 0]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

In your original suggestion, you also had vector bases as part of the arguments. I think these would still be needed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, I just wanted to keep it simple and generic for now, but expressing your coordinates in a new basis requires you to simply rotate them (which is the purpose of the rotation_matrix function).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe thinking about how one would use this in a notebook would be a good way to figure out the API.

You have a disk somewhere in your simulation. You want to basically rotate everything so that the z axis is aligned with the angular momentum (is that all you need to do?)

You could have the following steps:

  • find angular momentum vector
  • rotate all coords and vectors into a new basis (still in cartesian coords)
  • then access the radial or angular components of your vectors (this could be done via the .r and .theta properties?

The problem with just having .r property on a vector is that it works for position vectors, but not for other vectors such as velocity. If you want the radial velocity, you need to take the scalar product of the velocity vector with the position vector, so the conversion needs both position vectors and velocity vectors as inputs.

Maybe that is an argument for having functions that turn a whole Datagroup into spherical or cylindrical components?
Something like

spherical_hydro = osyris.spatial.get_spherical_components(data['hydro'])

Which would return a new Datagroup (i think it might be safer to return a new group rather than doing operations in-place).

But then the get_spherical_components also needs the data['amr']['position'] array...
Not sure if sending the entire Dataset to be converted is good, because you may be doubling your RAM usage.

Maybe the function should just accept a single Array, and an optional positions argument, which could just be None for computing the spherical components of the cell positions?

Copy link
Collaborator Author

@Adnan-Ali-Ahmad Adnan-Ali-Ahmad Feb 23, 2022

Choose a reason for hiding this comment

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

You have a disk somewhere in your simulation. You want to basically rotate everything so that the z axis is aligned with the angular momentum (is that all you need to do?)

Yes, that's all there is to it.

The problem with just having .r property on a vector is that it works for position vectors, but not for other vectors such as velocity. If you want the radial velocity, you need to take the scalar product of the velocity vector with the position vector, so the conversion needs both position vectors and velocity vectors as inputs.

Yeah that's what's been causing me a bit of a headache. It's difficult to come up with a clean API for something like this.

Not sure if sending the entire Dataset to be converted is good, because you may be doubling your RAM usage.

That is why in #52 I suggested that we make the coordinate system an attribute of the Dataset. That way with data.transform(system="spherical", basis="top"), you do not return a new dataset, but simply convert all components in it to spherical and keep the old basis stored somewhere so that the user can revert his changes. However, this solution is going to be quite costly in development time.

What I suggest is to have a get_spherical_components function that takes a Dataset as the argument but also a dictionary containing all the groups and arrays that the user would like to convert. Something like:

osyris.spatial.get_spherical_components(data, {"amr":"position", "hydro": ["velocity", "B_field"]})

It should be easier in this case to make a few checks on the user's request (eg. if they request spherical velocity components, we can access the position array since the whole dataset has been passed as an argument).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I forgot to say that this suggestion would return a new Dataset containing all the converted coordinates and .r, .theta and .phi components for each array.

Eg. :

spherical_coords = osyris.spatial.get_spherical_components(data, {"amr":"position", "hydro": ["velocity", "B_field"]})

spherical_coords["amr"]["position"].r # radius
spherical_coords["amr"]["position"].theta # colatitude
spherical_coords["amr"]["position"].phi # azimuth

spherical_coords[“hydro"]["velocity"].r # radial velocity
spherical_coords[“hydro"]["velocity"].phi # azimuthal velocity
etc...

And spherical_coords["amr"]["position"].x returns an attribute error.

"""
Converts cartesian components in 'position' to spherical components
Returns a list of osyris Arrays containing radius, colatitude and azimuth
"""
if isinstance(origin, Array):
centered_pos = position - origin
else:
centered_pos = position - Array(values=origin, unit=centered_pos.unit)

X = centered_pos[:, 0].values
Y = centered_pos[:, 1].values
Z = centered_pos[:, 2].values

radius = np.linalg.norm([X, Y, Z], axis=0)
colatitude = np.arctan2(np.linalg.norm([X, Y], axis=0), Z)

azimuth = np.arctan2(Y, X) + np.pi

radius = Array(values=radius, unit=position.unit, name='radius')
colatitude = Array(values=colatitude, unit=units('rad'), name='colatitude')
azimuth = Array(values=azimuth, unit=units('rad'), name='azimuth')

return radius, colatitude, azimuth


def cartesian_to_cylindrical(position, origin=[0, 0, 0]):
"""
Converts cartesian components in 'position' to cylindrical components
Returns a list of osyris Arrays containing radius, azimuth and elevation
"""
if isinstance(origin, Array):
centered_pos = position - origin
else:
centered_pos = position - Array(values=origin, unit=centered_pos.unit)

elevation = centered_pos[:, 2].values

radius = np.linalg.norm([centered_pos[:, 0].values, centered_pos[:, 1].values],
axis=0)
azimuth = np.arctan2(centered_pos[:, 1].values, centered_pos[:, 0].values)

radius = Array(values=radius, unit=centered_pos.unit, name='radius')
azimuth = Array(values=azimuth, unit=units('rad'), name='azimuth')
elevation = Array(values=elevation, unit=centered_pos.unit, name='elevation')

return radius, azimuth, elevation


def cylindrical_to_cartesian(radius, azimuth, elevation):
"""
Converts cylindrical components radius, azimuth and elevation to x,y,z
Returns an osyris array of shape (len(radius), 3) containing a stacked xyz
"""
x, y, z = (radius * np.cos(azimuth), radius * np.sin(azimuth), elevation)
xyz = np.vstack([x, y, z])
return Array(values=np.transpose(xyz).values, unit=radius.unit, name="position")