Skip to content

feat: Mock observation of brightness temperature#49

Open
DanielaBreitman wants to merge 46 commits intomainfrom
better_lc_noise
Open

feat: Mock observation of brightness temperature#49
DanielaBreitman wants to merge 46 commits intomainfrom
better_lc_noise

Conversation

@DanielaBreitman
Copy link
Copy Markdown
Contributor

@DanielaBreitman DanielaBreitman commented Sep 10, 2025

Closes #48.

@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

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 12, 2025

Codecov Report

❌ Patch coverage is 67.67677% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.95%. Comparing base (7480d86) to head (9fcd546).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/tuesday/core/instrument_models/noise.py 67.67% 17 Missing and 15 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
- Coverage   85.44%   82.95%   -2.50%     
==========================================
  Files          11       11              
  Lines         742      827      +85     
  Branches      188      206      +18     
==========================================
+ Hits          634      686      +52     
- Misses         44       62      +18     
- Partials       64       79      +15     
Flag Coverage Δ
unittests 82.95% <67.67%> (-2.50%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@steven-murray steven-murray left a comment

Choose a reason for hiding this comment

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

OK, I got through most of it and it looks decent, but got super confused on some important stuff. When that gets cleared up I'll have a better look at the example notebooks.

Comment on lines +289 to +290
sigma is set to zero for uv cells with less than
this number of baselines.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Technically, I think sigma should be set to np.inf... if in the end we do want to do inverse-variance weighting, np.inf will make this seamless.

@steven-murray
Copy link
Copy Markdown
Member

@nikos-triantafyllou and @DanielaBreitman I made some substantial updates on this PR, adding a lot of documentation and also changing some of the function names to be more clear.

@nikos-triantafyllou I'd be grateful if you could have another play with it -- the main example notebook has been updated to give you a head-start. But it only uses artificial lightcones, not realistic ones, so it would be good to have a look at those to catch any obvious errors.

Also, you'll need rasg-affiliates/21cmSense#215 to run this.

@steven-murray steven-murray changed the title feat: Issue #48 feature 1 feat: Mock observation of brightness temperature Mar 22, 2026
@review-notebook-app
Copy link
Copy Markdown

review-notebook-app bot commented Mar 29, 2026

View / edit / reply to this conversation on ReviewNB

DanielaBreitman commented on 2026-03-29T14:27:00Z
----------------------------------------------------------------

Formatting (:mod:)


@review-notebook-app
Copy link
Copy Markdown

review-notebook-app bot commented Mar 29, 2026

View / edit / reply to this conversation on ReviewNB

DanielaBreitman commented on 2026-03-29T14:27:00Z
----------------------------------------------------------------

Formatting


@review-notebook-app
Copy link
Copy Markdown

review-notebook-app bot commented Mar 29, 2026

View / edit / reply to this conversation on ReviewNB

DanielaBreitman commented on 2026-03-29T14:27:01Z
----------------------------------------------------------------

Formatting (##)


@review-notebook-app
Copy link
Copy Markdown

review-notebook-app bot commented Mar 29, 2026

View / edit / reply to this conversation on ReviewNB

DanielaBreitman commented on 2026-03-29T14:27:02Z
----------------------------------------------------------------

Formatting (##)


@review-notebook-app
Copy link
Copy Markdown

review-notebook-app bot commented Mar 29, 2026

View / edit / reply to this conversation on ReviewNB

DanielaBreitman commented on 2026-03-29T14:27:02Z
----------------------------------------------------------------

Extra \ after "chosen at random"


Copy link
Copy Markdown
Contributor Author

@DanielaBreitman DanielaBreitman left a comment

Choose a reason for hiding this comment

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

Very minor comments. Main thing to fix is tests. Besides that, things seem good (though I can't approve my own PR...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does this file actually belong in this PR?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hahaha no

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we should include the comparison with 21cmsense as part of the testing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Though this could be put into a new issue as part of improving testing in general as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, it takes a little bit of time to run -- I think it would be good as a test, but should be another PR.

dependencies = [
"21cmfast",
"21cmfast>=4.1.0",
"21cmsense>=2.0.0",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Needs to be updated for new 21cmsense version

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep, will need to merge that PR and make a release before merging this!

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.

[Feature Req.] Produce mock observation of brightness temperature.

3 participants