feat: Add coordinate_system parameter to compute_tangential_and_cross_components and _compute_lensing_angles functions #624
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…_components and _compute_lensing_angles functions
…ngles_flatsky function
…astropy function and fix bug in coordinate system conversion bug in compute_lensing_angles_astropy
|
@marina-ricci hey, I think the PR is ready for a review! |
… in _compute_lensing_angles_flatsky and updated tests
|
Since @hsinfan1996 mentioned that it was better to keep things consistent between _compute_lensing_angles_flatsky and _compute_lensing_angles_astropy, it may be better to revert the last commit. |
Yeah, I am sorry. I was thinking that the calculation was easier by flipping the sign of |
… systems in _compute_lensing_angles_flatsky and updated tests" This reverts commit 4a1faa6.
|
I asked @caioolivv to change the notebooks and add an example in the mock_data notebook for the two definitions. @caioolivv let me know when the PR is ready for review ! |
|
Everything's ready @marina-ricci ! |
marina-ricci
requested changes
Jul 17, 2024
There was a problem hiding this comment.
Thank you @caioolivv for this important PR! Since the coordinate system is an important but often missed subtlety, we should clearly highlight it. Hence I suggest :
- to add a references ( like papers) for the definitions in the code (in comments) and in the notebooks ;
- to highlights the differences between the cordinate systyems and profiles in
demo_dataops_functionality.ipynb - to expand also the
demo_mock_cluster.ipynbnotebook.
…rom 'pixel'/'sky' to 'euclidean'/'celestial'
|
Ready for review @marina-ricci |
|
@caioolivv I think there is a typo in |
|
Sorry @marina-ricci , was testing the paramenter validation and forgot to go back to the original value. |
|
Ok, everything seems good now ! |
marina-ricci
approved these changes
Jul 18, 2024
m-aguena
pushed a commit
that referenced
this pull request
Jul 18, 2024
…_components and _compute_lensing_angles functions (#624) * feat: Add coordinate_system parameter to compute_tangential_and_cross_components and _compute_lensing_angles functions * Set coordinate_system option during creation of GalaxyCluster object * Added option to choose coordinate system for generated mock catalog * Added coordinate_system validation and extra tests * Changed example notebook to document new coordinate_system option * uodated tag to 1.12.4 --------- Co-authored-by: Marina Ricci <[email protected]>
cavestruz
added a commit
that referenced
this pull request
Jul 19, 2024
…effects with instructions to use example data (#629) * feat: Add coordinate_system parameter to compute_tangential_and_cross_components and _compute_lensing_angles functions * Update documentation to account for new coordinate_system option * feat: Add coordinate_system conversion test to test_compute_lensing_angles_flatsky function * Add coordinate_system conversion test to test_compute_lensing_angles_astropy function and fix bug in coordinate system conversion bug in compute_lensing_angles_astropy * Set coordinate_system option during creation of GalaxyCluster object * adding a new notebook and data * test * new notebook to test coordinate systems and shear measurements * Removed debuggin print in test_dataops.py * editing README for the data availability * moved tomomi's notebook to examples * added prelude to Tomomi's notebook * Fixed typo and set default coordinate_system to "pixel" * Added option to choose coordinate system for generated mock catalog * Added ellipticity coordinate system conversion tests to test_galaxycluster.py and test_mockdata.py * Added documentation to other test cases in Tomomi's notebook. Still need to incorporate Caio's added functionality for sky vs. pixel coordinates in CLMM * Fixed formatting * starting to work through and document example of lensing signal calculation of HSCY3 cluster without CLMM, but need to change colossus dependency because it is not easy to install on NERSC * Finished baseline documentation, including changing colossus cosmology dependency in notebook to the astropy one so this is easier to run on NERSC * minor change to clarify sections of notebook * added kwargs to GalaxyCluster instance definition in preparation for re-installing latest CLMM with Caio's added coordinate_system kwargs * Updated logic for lensing angle conversion between coordinate systems in _compute_lensing_angles_flatsky and updated tests * Revert "Updated logic for lensing angle conversion between coordinate systems in _compute_lensing_angles_flatsky and updated tests" This reverts commit 4a1faa6. * Added coordinate_system validation and extra tests * Changed example notebook to document new coordinate_system option * examples/test_coordinate.ipynb has additional minor markdown detail * test_coordinate.ipynb now runs from top to bottom using Caio's coordinate system implementation! * adding additional docstrings to Tomomi's from-scratch calculation of lensing from HSC data * refactoring code into functions to make loop over cluster lenses more readable and some renaming of variables, e.g. sum to profile_sum, to avoid name space problems * made additional refactoring changes, but refactored functions to calculate angles between source and lens, etc. likely broke something in the background selection (now empty) * some of the refactoring incorporated with pre-broken code * cleaned up the loop over rbins * In line comments for portions of calculation * modified reverted commit to when the functions did not break, added in documentation and light refactoring from latest commit * renamed the notebook for consistency * docs/doc-config.ini now includes the demo_coordinate_system notebook * feat: Add coordinate_system parameter to compute_tangential_and_cross_components and _compute_lensing_angles functions (#624) * feat: Add coordinate_system parameter to compute_tangential_and_cross_components and _compute_lensing_angles functions * Update documentation to account for new coordinate_system option * feat: Add coordinate_system conversion test to test_compute_lensing_angles_flatsky function * Add coordinate_system conversion test to test_compute_lensing_angles_astropy function and fix bug in coordinate system conversion bug in compute_lensing_angles_astropy * Set coordinate_system option during creation of GalaxyCluster object * Removed debuggin print in test_dataops.py * Fixed typo and set default coordinate_system to "pixel" * Added option to choose coordinate system for generated mock catalog * Added ellipticity coordinate system conversion tests to test_galaxycluster.py and test_mockdata.py * Fixed formatting * Updated logic for lensing angle conversion between coordinate systems in _compute_lensing_angles_flatsky and updated tests * Revert "Updated logic for lensing angle conversion between coordinate systems in _compute_lensing_angles_flatsky and updated tests" This reverts commit 4a1faa6. * Added coordinate_system validation and extra tests * Changed example notebook to document new coordinate_system option * Fixed all the documentation for coordinate_system and changed names from 'pixel'/'sky' to 'euclidean'/'celestial' * Added reference for defitions of coordinate systems * Fixed bug in coordinate system conversion in mock data generation * Test ValueError raise for coordinate_system in test_mockdata.py * Added more explanations about ellipticity coordinate system * Fixed typo * uodated tag to 1.12.4 --------- Co-authored-by: Marina Ricci <[email protected]> * removed a character in a markdown cell that tests complained about in making docs * swapped pixel/euclidean and sky/celestial in notebook * reverted two files to try to match main * removed outputs in the notebook * removed OTHER notebook outputs * remove empty cells in notebooks * made minor edits in response to review, primarily with generalizing file paths and removing comments or redundant text in the markdown * incorporating original demo_mock_cluster.ipynb from main to remove changes to the file in this branch that should not have been committed * modified the Oguri example to also show incorrect coordinate assumption as noted by Caio * got rid of dollar signs inside markdown code snippet env hoping this will get the checks passed * added review contributions line :-) * added a note for the CosmoDC2 clusters so the user is aware to not necessarily expect a strong signal in center regions * removed extra cell at end * removed rogue savefig * clarified why there might be less signal in the TXPipe outputted CosmoDC2 thing * Formatted demo_coordinate_system_datasets.ipynb notebook --------- Co-authored-by: Caio Lima de Oliveira <[email protected]> Co-authored-by: Camille Avestruz <[email protected]> Co-authored-by: Marina Ricci <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add option to choose coordinate system of ellipticity components.