Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Subdomain extraction #86
Changes from 2 commits
41c1bae
bf66a73
7fe084a
2a8618d
86c9e21
5a5acb0
f959133
28dff7f
fe892fc
e179dcf
9711bea
7ca0cbc
a0071b3
8a26e2b
205adf4
a4800cb
509713c
e2018f0
3af9990
015d006
8faa3ff
81ef3e5
4be4096
a88c647
2d6b805
b5c3d6e
668789a
cd909e7
6fd8903
98b8702
16e522a
7896bc5
398962c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.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
).There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).There was a problem hiding this comment.
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:
.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
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 thedata['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 beNone
for computing the spherical components of the cell positions?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's all there is to it.
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.
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).
There was a problem hiding this comment.
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. :
And
spherical_coords["amr"]["position"].x
returns an attribute error.