Skip to content

Conversation

@jocelyn-mcmahon-16
Copy link

Upload of new galaxy clusters modeling notebook based on the work of Mahler et al. 2023 on the SMACS J0723.3-7237. Also added the relevant data files (arcs.dat, galcat.cat, input.par) needed to run the notebook.

…327 Mahler et al. 2023 data) and the data files

needed to run the notebook.
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -0,0 +1,1965 @@
{
Copy link
Collaborator

@sibirrer sibirrer Apr 30, 2025

Choose a reason for hiding this comment

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

might need more top-level information. Indicate what products are being used for this notebook, perhaps even in what format


Reply via ReviewNB

@@ -0,0 +1,1965 @@
{
Copy link
Collaborator

@sibirrer sibirrer Apr 30, 2025

Choose a reason for hiding this comment

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

do we still need these descriptions? Try not to duplicate too many feature of another notebook but instead link to that other notebook (easier to maintain fewer information per notebook)


Reply via ReviewNB

@@ -0,0 +1,1965 @@
{
Copy link
Collaborator

@sibirrer sibirrer Apr 30, 2025

Choose a reason for hiding this comment

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

here I would provide more text, the file name and structure, perhaps even the link to the paper


Reply via ReviewNB

@@ -0,0 +1,1965 @@
{
Copy link
Collaborator

@sibirrer sibirrer Apr 30, 2025

Choose a reason for hiding this comment

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

Line #33.    for i in range(len(gal_data_deg)):

you have many different for-loops here operating on the same part (coordinates). Might be better to do only one for-loop and do all the things at once, perhaps having a definition that does it


Reply via ReviewNB

@@ -0,0 +1,1965 @@
{
Copy link
Collaborator

@sibirrer sibirrer Apr 30, 2025

Choose a reason for hiding this comment

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

Line #107.    #     :param center_y: center of halo (in angular units)

do we need all the prints here? It's hard to read it anyways


Reply via ReviewNB

@sibirrer sibirrer self-requested a review May 7, 2025 16:31
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

Successfully merging this pull request may close these issues.

2 participants