Skip to content

Fix half plane issues - #94

Open
steven-murray wants to merge 5 commits into
better_lc_noisefrom
fix-half-plane-issues
Open

Fix half plane issues#94
steven-murray wants to merge 5 commits into
better_lc_noisefrom
fix-half-plane-issues

Conversation

@steven-murray

Copy link
Copy Markdown
Member

This is some work I'm doing to see if I can fix the half-plane vs full-plane issues. Just a draft for now as I'm working on it between this and the 21cmSense PR.

@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

@steven-murray
steven-murray changed the base branch from main to better_lc_noise June 7, 2026 17:35
@steven-murray
steven-murray marked this pull request as ready for review June 8, 2026 02:00

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @steven-murray, your pull request is larger than the review limit of 150000 diff characters

@steven-murray steven-murray self-assigned this Jun 8, 2026
@steven-murray steven-murray added API Breaking Breaking Changes type: bug Something isn't working priority: high High priority labels Jun 8, 2026
@steven-murray

Copy link
Copy Markdown
Member Author

@nikos-triantafyllou this is finally ready for review -- there were a couple of small bugs on the 21cmSense side which are now fixed in a PR there (this branch adds the specific 21cmSense branch as a dependency... once you check this and give the all clear, I'll pull the trigger on the 21cmSense PR and update tuesday to depend on an actual 21cmSense version again).

@steven-murray

Copy link
Copy Markdown
Member Author

@nikos-triantafyllou it looks like it will fail tests until I merge and release the new 21cmSense, but you should be able to install it with uv properly locally (the CI fails because it uses miniconda+pip to handle deps rather than uv, which is fine usually but it doesn't pick up that 21cmSense is pointed to a git url rather than pypi). So, go ahead and look at it and test it out, and if you're satisfied, I'll merge upsteream.

BTW this does not attempt to fix #104 at all -- once this is merged into better_lc_noise, we can work on a fix from there.

@review-notebook-app

review-notebook-app Bot commented Jun 18, 2026

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

nikos-triantafyllou commented on 2026-06-18T11:55:38Z
----------------------------------------------------------------

Line #7.    from py21cmsense import Observation, Observatory, convert_half_to_full_uv_plane

Maybe do the same for 21cmsense to make it clear where the functions are coming from. Not a priority though, can be another PR.


@review-notebook-app

review-notebook-app Bot commented Jun 18, 2026

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

nikos-triantafyllou commented on 2026-06-18T11:55:39Z
----------------------------------------------------------------

Line #3.    lc_with_noise = observe_lightcone(

Nice clean up!


@nikos-triantafyllou nikos-triantafyllou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just a small change in the testing file, I assume you wanted to check the imaginary part as well.

- name: Install Non-Python Dependencies
run: |
conda install fftw gsl
conda install pip fftw gsl numba compilers pkg-config

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Where do we use numba?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We don't explicitly but I think one of our deps does and it's easier to install with conda in this case

Comment thread tests/test_lc_noise.py Outdated
Co-authored-by: Nikos Triantafyllou <nikolaos.triantafyllou@sns.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API Breaking Breaking Changes priority: high High priority type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants