Skip to content

add band checks to knn#17

Open
sschmidt23 wants to merge 6 commits into
mainfrom
issue/16/errmsg
Open

add band checks to knn#17
sschmidt23 wants to merge 6 commits into
mainfrom
issue/16/errmsg

Conversation

@sschmidt23

Copy link
Copy Markdown
Collaborator

Addresses #16 by adding a couple of checks on the bands, ref_band, and mag_limits values. My local pytest is acting up, so I don't know if the tests work or not, I'll create a draft PR to run them here.

Code Quality

  • My code follows the code style of this project
  • I have written unit tests or justified all instances of #pragma: no cover; in the case of a bugfix, a new test that breaks as a result of the bug has been added
  • My code contains relevant comments and necessary documentation for future maintainers; the change is reflected in applicable demos/tutorials (with output cleared!) and added/updated docstrings use the NumPy docstring format
  • Any breaking changes, described above, are accompanied by backwards compatibility and deprecation warnings

@sschmidt23

Copy link
Copy Markdown
Collaborator Author

and, my mouse slipped and I created areal PR, oops.

@sschmidt23 sschmidt23 requested review from aimalz and hangqianjun June 17, 2024 20:56
@sschmidt23

Copy link
Copy Markdown
Collaborator Author

added some linting changes that make the additions looks more substantial than they really are, tests now run ok and should give more informative error messages. Tagging both @aimalz and @hangqianjun for reviewers.

@hangqianjun hangqianjun left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for getting back to you for so long!! I somehow skipped this PR. It looks good! Some of these tests may be included in a set of more generic column name tests in tables_io.

@sschmidt23

Copy link
Copy Markdown
Collaborator Author

@hangqianjun it looks like you are working on essentially the same checks in tables_io, should we maybe just abandon this PR rather than merging it and just let tables_io be responsible for the checks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants