Move assert_quantity_allclose import to lower level in bls.core#7932
Move assert_quantity_allclose import to lower level in bls.core#7932pllim wants to merge 1 commit intoastropy:masterfrom
Conversation
|
Hi there @pllim 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
| The other results object to compare. | ||
|
|
||
| """ | ||
| from ...tests.helper import assert_quantity_allclose |
There was a problem hiding this comment.
Is the assert_allclose actually meant to be used by users or is it just for the tests? (maybe @dfm can clarify?)
If it's meant to be used by users, I wonder if we should instead import from astropy.units.quantity import _unquantify_allclose_arguments and use np.assert_allclose here? (to avoid the pytest dependency)
There was a problem hiding this comment.
Usually, "assert" is used in testing. In a non-testing case, maybe use is_allclose that returns a boolean instead, or maybe that ship has sailed now...?
There was a problem hiding this comment.
it hasn't sailed at all, the bls module is only in dev so any fixes should be done now
There was a problem hiding this comment.
actually, @dfm, I wonder whether we need this convenience at all?
There was a problem hiding this comment.
Hi team! This is just for testing and it's not meant as a user facing method... perhaps we should move it to the tests script anyways?
There was a problem hiding this comment.
@dfm - thanks for confirming - indeed this should definitely be moved inside the tests directory in this case.
|
In #7938 I made an alternative to this that moves the whole thing over into the testing suite. Based on #7932 (comment) I think that's the way to go? |
This is because
assert_quantity_allcloserequirespytestbutblsin general can be used withoutpytest. In other words,pytestshould only be required when one runs tests.blswas added in #7391.xref astropy/astropy-benchmarks#68