You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
We recently merged a PR to specify the coordinate system for the shear, but it lacks some functionalities.
First, it should be explained in details in a notebook, so that this bug would have been found. E.g : showing a profile taken with an incorrect coordinate system and showing all intermediate steps.
At the moment, the keyword is missing from compute_tangential_and_cross_components. You can pass it to the Galaxy Cluster object, but it does not imply that it will be used (this should be avoided).
Also, we might consider adding a simple way to convert between the two coordinate, once compute_tangential_and_cross_components is run (not having to rerun all steps).
(Note : I discovered this while trying to implement in TXPipe and seeing no changes).
The text was updated successfully, but these errors were encountered:
Hey @marina-ricci, just to be sure I understand what's the problem: GalaxyCluster.compute_tangential_and_cross_components() should have an option to pass in the coordinate system or should it explain that GalaxyCluster.coordinate_system will be used (or a combination of both)?
Also I agree there should be a function to convert between them. Maybe GalaxyCluster.convert_coordinate_system()? In that case, should you call it and then call GalaxyCluster.compute_tangential_and_cross_components() again or just calling GalaxyCluster.convert_coordinate_system() should already compute new tangential and cross components?
Finally, there's the examples/demo_coordinate_system_datasets.ipynb notebook from another PR which shows the problems of using another coordinate system. Is that what you meant? In that case we could reference it in another notebook?
Hi @caioolivv ! I did not see that the examples/demo_coordinate_system_datasets.ipynb notebook was merged to main. I'm not sure it should stay here in it's current form.
These are my suggestions :
add warning message when GalaxyCluster (or compute_tangential_xxx, see second point) object is initiated without specifying the coordinate system. Like "no coordinate system provided, taking euclidean as default"
put coordinate system as a keyword for compute_tangential_and_cross_components, from data_ops or as a GalaxyCluster method. Maybe not necessary to have it as a GalaxyCluster keyword because it may be confusing as it is not necessarily propagated.
create a simple notebook to highlight this.
I created a branch with a notebook to show the problem (it's not fully documented but should be understandable)
We recently merged a PR to specify the coordinate system for the shear, but it lacks some functionalities.
First, it should be explained in details in a notebook, so that this bug would have been found. E.g : showing a profile taken with an incorrect coordinate system and showing all intermediate steps.
At the moment, the keyword is missing from
compute_tangential_and_cross_components
. You can pass it to theGalaxy Cluster
object, but it does not imply that it will be used (this should be avoided).Also, we might consider adding a simple way to convert between the two coordinate, once
compute_tangential_and_cross_components
is run (not having to rerun all steps).(Note : I discovered this while trying to implement in TXPipe and seeing no changes).
The text was updated successfully, but these errors were encountered: