Skip to content

Modify sig_uv calculation by removing scaling factor#58

Closed
YilongZhang-Cosmo wants to merge 2 commits into
21cmfast:better_lc_noisefrom
YilongZhang-Cosmo:better_lc_noise
Closed

Modify sig_uv calculation by removing scaling factor#58
YilongZhang-Cosmo wants to merge 2 commits into
21cmfast:better_lc_noisefrom
YilongZhang-Cosmo:better_lc_noise

Conversation

@YilongZhang-Cosmo

Copy link
Copy Markdown

Removed unnecessary scaling factor of 1e6 from the calculation of sig_uv.

@DanielaBreitman DanielaBreitman self-requested a review November 11, 2025 13:21
@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@review-notebook-app

review-notebook-app Bot commented Nov 26, 2025

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

DanielaBreitman commented on 2025-11-26T15:51:53Z
----------------------------------------------------------------

Why can't we just import this function from tuesday?


@review-notebook-app

review-notebook-app Bot commented Nov 26, 2025

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

DanielaBreitman commented on 2025-11-26T15:51:54Z
----------------------------------------------------------------

Line #2.    #uv_coverage = np.zeros((lc_shape[0],lc_shape[0],len(freqs)))#Nu, Nv, Nfreqs

Just delete this.


@review-notebook-app

review-notebook-app Bot commented Nov 26, 2025

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

DanielaBreitman commented on 2025-11-26T15:51:54Z
----------------------------------------------------------------

No need to keep this I think.


@review-notebook-app

review-notebook-app Bot commented Nov 26, 2025

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

DanielaBreitman commented on 2025-11-26T15:51:55Z
----------------------------------------------------------------

This also maybe no need to have.


@review-notebook-app

review-notebook-app Bot commented Nov 26, 2025

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

DanielaBreitman commented on 2025-11-26T15:51:56Z
----------------------------------------------------------------

Also no need to have this.


@review-notebook-app

review-notebook-app Bot commented Nov 28, 2025

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

steven-murray commented on 2025-11-28T01:04:15Z
----------------------------------------------------------------

Please make the bandwidth a reasonable number like 8 MHz


@review-notebook-app

review-notebook-app Bot commented Nov 28, 2025

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

steven-murray commented on 2025-11-28T01:04:16Z
----------------------------------------------------------------

Why did this change so much?


@review-notebook-app

review-notebook-app Bot commented Nov 28, 2025

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

steven-murray commented on 2025-11-28T01:04:17Z
----------------------------------------------------------------

You can have a look at my new noteboook on the better_lc_noise branch -- I pass in the sigma_uv as weights here.


@steven-murray

Copy link
Copy Markdown
Member

Closing this in favour of the changes as made in #49

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