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-493: add module for Gaussian random fields #494

Merged
merged 19 commits into from
Feb 13, 2025
Merged

gh-493: add module for Gaussian random fields #494

merged 19 commits into from
Feb 13, 2025

Conversation

ntessore
Copy link
Collaborator

Move functionality from gaussiancl into a new module glass.grf. Transformations of Gaussian random fields can be carried out on all supported array types.

This is still a draft: core functionality in glass still needs to be expressed in terms of the new module.

Closes: #493

@ntessore
Copy link
Collaborator Author

ntessore commented Jan 30, 2025

Btw, I have switched off a lot of annoying linter rules.

Moved this to a new issue: #499

@ntessore ntessore marked this pull request as ready for review February 1, 2025 16:21
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.

Thanks, @ntessore! Looks almost ready to me! Sorry for the long review 😬 but see my suggestions and questions below -

@Saransh-cpp Saransh-cpp added enhancement New feature or request api An (incompatible) API change labels Feb 4, 2025
@ntessore ntessore requested a review from Saransh-cpp February 7, 2025 20:31
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.

Thanks, @ntessore! Some minor formatting below. I will open up issues for the remaining bits.

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.

I've looked through and made various comments.

My main concern is that this is a massive PR, which I really think should be split up.

There have also been design choices which should be discussed, or just done in separate PRs

This PR also undoes a lot of the documentation work done by #381. I think it would be a shame to introduce a lot of code not following that.

@paddyroddy
Copy link
Member

Tests are broken FYI

@ntessore
Copy link
Collaborator Author

Implemented changes from review @paddyroddy and @Saransh-cpp. Tests are broken on main.

@paddyroddy
Copy link
Member

Implemented changes from review @paddyroddy and @Saransh-cpp. Tests are broken on main.

They're working for me? And in the UI

@ntessore
Copy link
Collaborator Author

Isn't it nox itself that's erroring out, so nothing to do with this PR?

@paddyroddy
Copy link
Member

Isn't it nox itself that's erroring out, so nothing to do with this PR?

Ah, yes, you're correct. @Saransh-cpp you are the one in support of nox. This is the same error I saw before — although I fixed it in #471. I really think we should switch to tox, as attempted here #476. Consistent bugs are just not acceptable.

@Saransh-cpp
Copy link
Member

Saransh-cpp commented Feb 13, 2025

I am okay with switching to tox, but the bug was more of a GH Action issue this time. The original nox issue was fixed in #471, but the PR did not fix it properly and introduced another issue with caching which popped up now (I missed it in my review 😬).

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.

Thanks, @ntessore! I believe we've created appropriate issues about the conventions/rules changed in this PR, so this should be good to go. I'll wait for @paddyroddy's review before merging.

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.

LGTM

@ntessore
Copy link
Collaborator Author

Thanks both, and sorry for the big & complicated PR. These are accumulated changes that would have been difficult to split up again into smaller chunks.

@paddyroddy paddyroddy merged commit 0220019 into main Feb 13, 2025
11 checks passed
@paddyroddy paddyroddy deleted the nt/grf branch February 13, 2025 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api An (incompatible) API change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New mechanism for Gaussian random field transformations
3 participants