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

gh-504: deprecate old generator functions #523

Merged
merged 13 commits into from
Feb 21, 2025
Merged

gh-504: deprecate old generator functions #523

merged 13 commits into from
Feb 21, 2025

Conversation

ntessore
Copy link
Collaborator

Deprecates the old random field generator functions in favour or glass.generate(). Internally, reimplements all old functionality in terms of the new functions, and removes the dependency on the gaussiancl package.

Still a draft as I want to check that the examples don't change under the reimplementation, before also updating them to use the new functions.

Closes: #504

@ntessore
Copy link
Collaborator Author

Sphinx error doesn't happen for me locally 😞

@ntessore
Copy link
Collaborator Author

Marking as ready so examples are built, but not ready.

@ntessore ntessore marked this pull request as ready for review February 19, 2025 12:01
@ntessore
Copy link
Collaborator Author

Apparently, sphinx-autodoc-typehints is broken: tox-dev/sphinx-autodoc-typehints#523

@paddyroddy
Copy link
Member

Apparently, sphinx-autodoc-typehints is broken: tox-dev/sphinx-autodoc-typehints#523

Was literally just about to send that too

@ntessore ntessore marked this pull request as draft February 19, 2025 12:51
@ntessore ntessore marked this pull request as ready for review February 19, 2025 14:22
@ntessore ntessore requested review from paddyroddy and Saransh-cpp and removed request for paddyroddy February 19, 2025 15:21
@Saransh-cpp
Copy link
Member

I'll take over this PR.

@Saransh-cpp Saransh-cpp self-assigned this Feb 19, 2025
@Saransh-cpp Saransh-cpp marked this pull request as draft February 19, 2025 15:36
@Saransh-cpp Saransh-cpp added this to the v2025.1 milestone Feb 19, 2025
@Saransh-cpp Saransh-cpp added maintenance Maintenance: refactoring, typos, etc. api An (incompatible) API change labels Feb 19, 2025
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

@paddyroddy this should be ready now!

@@ -199,44 +222,6 @@ def multalm(
return out


def transform_cls(
Copy link
Member

Choose a reason for hiding this comment

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

Should this be deprecated before removal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This isn't public API, it was exported by mistake.

@Saransh-cpp Saransh-cpp marked this pull request as ready for review February 19, 2025 16:39
@Saransh-cpp Saransh-cpp self-requested a review February 19, 2025 16:39
@paddyroddy
Copy link
Member

@paddyroddy this should be ready now!

Can't render the notebook diffs

@paddyroddy
Copy link
Member

and removes the dependency on the gaussiancl package.

and introduces transformcl dependency

@paddyroddy
Copy link
Member

and introduces transformcl dependency

Although having a look gaussiancl depends on it anyway

Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

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

Have run and diffed things locally. Looks good to me.

glass/fields.py Outdated
If all gls are empty.

"""
"""Iteratively sample Gaussian random fields (internal use)."""
Copy link
Member

Choose a reason for hiding this comment

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

Why get rid of the docstring? Just because its internal doesn't mean it shouldn't have one

Copy link
Member

Choose a reason for hiding this comment

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

Maybe @ntessore can answer this better, but the docstring was moved to generate in another PR -

glass/glass/fields.py

Lines 773 to 817 in 84dff24

def generate(
fields: Fields,
gls: Cls,
nside: int,
*,
ncorr: int | None = None,
rng: np.random.Generator | None = None,
) -> Iterator[NDArray[Any]]:
"""
Sample random fields from Gaussian angular power spectra.
Iteratively sample HEALPix maps of transformed Gaussian random
fields with the given angular power spectra *gls* and resolution
parameter *nside*.
The random fields are sampled from Gaussian random fields using the
transformations in *fields*.
The *gls* array must contain the angular power power spectra of the
Gaussian random fields in :ref:`standard order <twopoint_order>`.
The optional number *ncorr* limits how many realised fields are
correlated. This saves memory, as only *ncorr* previous fields are
kept.
Parameters
----------
fields
Transformations for the random fields.
gls
Gaussian angular power spectra.
nside
Resolution parameter for the HEALPix maps.
ncorr
Number of correlated fields. If not given, all fields are
correlated.
rng
Random number generator. If not given, a default RNG is used.
Yields
------
x
Sampled random fields.
"""

I wouldn't mind having it repeated (given that they are not identical) here.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I spotted it had moved

Copy link
Member

Choose a reason for hiding this comment

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

Have added the docstring back. Let me know if this should be removed.

@Saransh-cpp Saransh-cpp merged commit 97477f9 into main Feb 21, 2025
16 checks passed
@Saransh-cpp Saransh-cpp deleted the nt/504 branch February 21, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api An (incompatible) API change maintenance Maintenance: refactoring, typos, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate prior random field generators
3 participants