Skip to content

Lyman-alpha cross-correlation#210

Open
tristpinsm wants to merge 24 commits intomainfrom
tpm/lyalpha
Open

Lyman-alpha cross-correlation#210
tristpinsm wants to merge 24 commits intomainfrom
tpm/lyalpha

Conversation

@tristpinsm
Copy link
Contributor

I'm about to start writing the paper up for this analysis, so I thought it would be good to also get the code reviewed and merged.

@tristpinsm tristpinsm requested review from jrs65 and sjforeman November 4, 2022 17:44
@tristpinsm tristpinsm force-pushed the tpm/lyalpha branch 2 times, most recently from 7c8599c to 836af4c Compare November 4, 2022 18:20
Copy link
Contributor

@jrs65 jrs65 left a comment

Choose a reason for hiding this comment

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

I'm still going, but here are some first round comments.

return data_exp


class GaussianNoiseExternal(task.SingleTask, random.RandomTask):
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this could be done by using the existing tasks GaussianNoise to generate a realisation and then MixData to combine them, rather than adding an extra task?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I guess so, I hadn't thought of using MixData

@tristpinsm tristpinsm force-pushed the tpm/lyalpha branch 3 times, most recently from 87942f3 to 2c38660 Compare November 24, 2022 01:30
@tristpinsm
Copy link
Contributor Author

OK, I've addressed most of Richard's comments now and also added some new tasks to perform the cross-correlations and make permutations of spectra.

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