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

adding knot size following disk profile #79

Closed
wants to merge 7 commits into from
Closed

Conversation

matroxel
Copy link
Collaborator

Update adds non-zero sizes to the knot component of the profile, fixed to 250pc. Also changes distribution of knot locations to follow the disk profile, rather than a gaussian. A comparison of the impact of the changes follows.

Screen Shot 2023-10-30 at 5 02 29 PM

Top: Version in main; Bottom: pull request update
Left: bulge component included; Right: Bulge component turned off.
(note, random seed not fixed between panels in test, so knot locations change)

@matroxel
Copy link
Collaborator Author

matroxel commented Nov 3, 2023

I've updated this pull request to include the calculation of n_knots as well and fix a bug where the random seed for the knot distribution was user-defined and not fixed, so different people using sky catalogs would typically end up with different knot positions.

If n_knots exists as a native attribute, it will use that value - otherwise it will use the new n_knots calculation, so if we want to add a more realistic fixed n_knot model later we can just add it to the catalogs and the default random code will be overwritten. Also enables backward compatibility with cosmodc2.

I couldn't figure out how to get a config-level entry inside the object classes, so the physical knot size (constant for all galaxies) is still hard-coded. I'm happy to change that to a config-able attribute if someone points me to how to get a value from the config and into e.g. base_object.

This should be fully compatible with current cosmodc2 objects and some things (like the random bug fix) should be applied to them no matter what, but still might be worth waiting to review this until after the diffsky additions are more advanced.

@matroxel matroxel closed this Nov 21, 2023
@matroxel matroxel deleted the u/troxel/knot_update branch November 29, 2023 21:50
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.

1 participant