Make magnetization direction required in public reduction_to_pole#684
Open
gaoflow wants to merge 1 commit into
Open
Make magnetization direction required in public reduction_to_pole#684gaoflow wants to merge 1 commit into
gaoflow wants to merge 1 commit into
Conversation
PR fatiando#661 (fixing fatiando#440) made the magnetization inclination/declination required in the reduction_to_pole_kernel and removed the code that filled in defaults when they were None. However, the public reduction_to_pole function was missed: it still declared magnetization_inclination=None and magnetization_declination=None and passed those straight to the kernel. As a result, the documented default usage reduction_to_pole(grid, inclination, declination) (omitting the magnetization direction) crashed deep inside the kernel with a cryptic 'np.radians(None)' TypeError instead of the intended behaviour of requiring the user to be explicit about the magnetization direction. Complete fatiando#661 on the public function: make the magnetization arguments required and drop the stale 'If None ...' docstring text (mirroring the kernel). Omitting them now raises a clear 'missing required argument' error at call time. Add a regression test. Relevant issues/PRs: fatiando#440, fatiando#661
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes #661 (which fixes #440).
Problem
#661 made the magnetization inclination/declination required in
reduction_to_pole_kerneland removed the code that filled in defaults when they wereNone. However, the publicreduction_to_polefunction in_transformations.pywas missed — it still declared:and passed those straight into the now-strict kernel. So the documented default usage (omitting the magnetization direction, as the docstring still described) crashes deep inside the kernel:
This is exactly the silent "induced-only magnetization" foot-gun that @leouieda wanted to remove in #440 — except now it is a crash with a confusing message rather than a clear, explicit requirement.
Fix
Complete #661 on the public function:
magnetization_inclination/magnetization_declinationrequired arguments (remove the=Nonedefaults).Omitting the magnetization direction now raises a clear error at call time:
Verification
reduction_to_poletests and gallery/user-guide examples already pass the magnetization angles explicitly (updated in Make the magnetization direction required in RTP #661), so they are unaffected.test_reduction_to_pole_magnetization_required) asserting the arguments are required; it fails onmain(the old crypticTypeErrormessage does not name the argument) and passes with this change.test/test_transformations.py+test/test_filters.py: 61 passed.ruff check/ruff format --checkclean.