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

Eliminate independently-defined LogNormalScatter class #746

Open
aphearin opened this issue May 7, 2017 · 3 comments
Open

Eliminate independently-defined LogNormalScatter class #746

aphearin opened this issue May 7, 2017 · 3 comments
Assignees
Milestone

Comments

@aphearin
Copy link
Contributor

aphearin commented May 7, 2017

Defining scatter in an independent class is a bit convoluted because the functionality is so simple. After some experimenting with building models, reliance on this class does not really improve convenience. It is probably better to entirely do away with this class and just have any stellar-to-halo-mass relation implement its own scatter. The downside is a mild amount of code duplication will be required, but I think this outweighed by the improved clarity and relaxed restrictions on future development.

It may be better to similarly refactor the stellar-to-halo-mass relations so that PrimGalPropModel is also entirely eliminated. @johannesulf or @duncandc - do either of you have an opinion on this? If it makes no difference to either of you, I'll probably just go ahead and eliminate both of these classes.

@aphearin aphearin added this to the next-release milestone May 7, 2017
@aphearin aphearin self-assigned this May 7, 2017
@johannesulf
Copy link
Contributor

I have no strong opinion on that. Generally, I agree with you on that issue. I didn't even know that such a class existed and built models without it.

@duncandc
Copy link
Contributor

duncandc commented May 8, 2017

I 100% agree with you both on this issue. In fact, I have already ditched the scatter class when building my own models. This is certainly a case of us being a little too class happy in the early days of halotools.

@aphearin
Copy link
Contributor Author

aphearin commented May 8, 2017

Great, thanks for weighing in. I'll definitely get rid of the scatter class.

I'll take a close look at getting rid of PrimGalpropModel class as well. The main feature that PrimGalpropModel had was to automatically provide the mc_ version of some mean relation such as <M*|Mh> defined by the user. In hindsight, that machinery seems unnecessary (and actually obfuscating).

@aphearin aphearin modified the milestones: v0.5, next-release May 8, 2017
@aphearin aphearin modified the milestones: v0.6, v0.5 May 13, 2017
@aphearin aphearin modified the milestones: v0.6, next-release Nov 26, 2017
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

3 participants