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

Merge magnetic forward functions for prisms #448

Merged
merged 10 commits into from
Jan 10, 2024
Merged

Conversation

santisoler
Copy link
Member

@santisoler santisoler commented Jan 6, 2024

Merge the two forward functions for magnetic fields of rectangular prisms into a single one named prism_magnetic that includes a new field argument, mimicking the interface of the gravity forward functions. Make the prism_magnetic` function to ask magnetization vectors as a tuple of three arrays, where each one of these arrays. Update tests accordingly. Update the user guide page about forward modelling of prisms.

Relevant issues/PRs:

Fixes #446 and fixes #447

Merge the two forward functions for magnetic fields of rectangular
prisms into a single one named `prism_magnetic` that includes a new
`field` argument, mimicking the interface of the gravity forward
functions. Update tests accordingly.
Make the `prism_magnetic` function to ask magnetization vectors as
a tuple of three arrays, where each one of these arrays.
@santisoler santisoler added this to the v0.7.0 milestone Jan 6, 2024
Ditch the usage of `prism_magnetic_component` in the forward modelling
pages, and change how the magnetization of the prisms is defined.
@santisoler santisoler marked this pull request as ready for review January 9, 2024 23:54
@santisoler
Copy link
Member Author

@indiauppal, I think this is ready to be merged. Would you like to review it?

No worries if you don't, and there's no rush to do so. But I think it would be a nice experience for you!

If you agree, here is a GitHub guide on how to make a review: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request

Let me know what do you think.

Copy link
Member

@indiauppal indiauppal left a comment

Choose a reason for hiding this comment

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

A couple of tiny changes in doc/user_guide/forward_modelling/prism.rst, but otherwise it's ready to be merged.
Thanks @santisoler for letting me review it!

santisoler and others added 3 commits January 10, 2024 09:13
Replace the `cast` argument in the private functions for the `shape` of
the expected output arrays. Extend the docstrings of those private
functions.
Co-authored-by: India Uppal <[email protected]>
@santisoler
Copy link
Member Author

Thanks @indiauppal for the review! Nice catches!

I've just pushed a few more changes: I filled the docstrings of the new private functions (which were a little bit scarse), and noticed that asking for the cast was hard to explain, so I changed it for the shape of the output arrays.

If CI doesn't fail, I'll merge this right away!

Thanks @santisoler for letting me review it!

No need to thank me, it's the other way around haha! BTW, don't feel you need to ask permission to review a PR that is interesting for you. Feel free to jump in and make the review!

@santisoler santisoler merged commit 11a62da into main Jan 10, 2024
20 checks passed
@santisoler santisoler deleted the magnetic-prism-update branch January 10, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants