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

Problem with coordinate implementation #635

Open
marina-ricci opened this issue Jul 22, 2024 · 2 comments
Open

Problem with coordinate implementation #635

marina-ricci opened this issue Jul 22, 2024 · 2 comments
Assignees

Comments

@marina-ricci
Copy link
Collaborator

marina-ricci commented Jul 22, 2024

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).

@caioolivv
Copy link
Collaborator

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?

@marina-ricci
Copy link
Collaborator Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants