Address outstanding technical debt issues.#12
Conversation
|
A pre-review question because I need to understand some context: It looks like you've addressed all of the requirements to satisfy closing the issues referenced in this PR. However, you don't appear to have used anything from other DESI packages from have already resolved the same issues. I'm particularly concerned that you do not test a range of versions of NumPy. We currently have examples in other packages of tests that cover a range from NumPy 1.22.x all the way to NumPy 2.3 (not every version, just a sampling of that range). Is there a plan or requirement that QuasarNP not follow the template of other DESI packages? |
|
PS, although tests are passing on this branch, there are warnings raised by the tests that can and should be fixed. These warnings could distract from more serious warnings. |
|
@weaverba137 QuasarNP was written outside the DESI ecosystem so was not originally designed to follow the same guidelines as the other DESI packages. My current fix for the Numpy 2.0 support is the "path of least resistence" so to speak, but if it is a strict necessity that QuasarNP move inline with other DESI packages then I feel that that is a broader discussion than just unit tests or technical debt. For example, the base package structure does not match other DESI packages, which place source code in a If we only want the structure of the unit tests to match that of the other DESI packages, I'm happy to make that change now as part of this PR. If there is a wider concern about QuasarNP's structure not matching that of other DESI packages then there is going to be a delay in implementing everything, since it will take more effort to ensure
Related question: are the actual package requirements/templates written down somewhere? When I created QuasarNP I don't think I ever had any guidance on what should and shouldn't be in a DESI package (and, once again, when it was written it wasn't intended to be a critical DESI package). Is the warning you are referring to the invalid escape sequence? I'll take care of that. |
|
It's OK if QuasarNP does not follow the DESI package layout/template, I just wanted to verify that. You may want to consider explicitly mentioning that in the README file for example if you haven't already. We have a product called "desitemplate". The test matrix of that package was updated recently to illustrate how to test a variety of version of versions of NumPy. So I think you can consider expanding your test matrix without changing other aspects of the package layout.
Yes, that's what I meant, thanx. |
|
@weaverba137 I've addressed the warning and implemented the testing matrix from desitemplate. |
weaverba137
left a comment
There was a problem hiding this comment.
Looks fine. Merge when ready.
| @@ -1,5 +1,6 @@ | |||
| # QuasarNP | |||
| [](https://quasarnp.readthedocs.io/en/latest/?badge=latest) | |||
| [](https://coveralls.io/github/desihub/QuasarNP?branch=technical_debt) | |||
There was a problem hiding this comment.
Don't forget to change the branch to main after this is merged.
This PR addresses the issues #9, #10, and #11.
@weaverba137 I am requesting a review mostly to verify that there are no other major outstanding technical debt issues that need to be resolved before a merge & close of the issues. Thanks.