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

Missing tests: is_block_positive #335

Merged
merged 7 commits into from
Dec 13, 2023
Merged

Conversation

purva-thakre
Copy link
Collaborator

@purva-thakre purva-thakre commented Dec 6, 2023

Description

Related to some uncovered lines in #241

Last line is still not covered by this PR.

Todos

Notable points that this PR has either accomplished or will accomplish.

  • TODO 1

Questions

  • Question1

Status

  • Ready to go

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2226c46) 97.2% compared to head (4940957) 97.4%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #335     +/-   ##
========================================
+ Coverage    97.2%   97.4%   +0.1%     
========================================
  Files         152     152             
  Lines        3046    3044      -2     
  Branches      726     725      -1     
========================================
+ Hits         2961    2965      +4     
+ Misses         50      48      -2     
+ Partials       35      31      -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@purva-thakre
Copy link
Collaborator Author

@vprusso Do you mind explaining the use-case scenario for these lines? I am struggling with defining the input matrix and dims for this case.

dim = np.array([dim, dim_xy / dim]) # pylint: disable=redefined-variable-type
if np.abs(dim[1] - np.round(dim[1])) >= 2 * dim_xy * np.finfo(float).eps:
raise ValueError(
"If `dim` is a scalar, it must evenly divide the length of the matrix."
)

@purva-thakre
Copy link
Collaborator Author

To do: Search for open ended discussions in published refs for below.

return RuntimeError(
"Unable to determine k-block positivity. Please consider increasing the relative tolerance or the effort level."
)

@vprusso
Copy link
Owner

vprusso commented Dec 8, 2023

@vprusso Do you mind explaining the use-case scenario for these lines? I am struggling with defining the input matrix and dims for this case.

dim = np.array([dim, dim_xy / dim]) # pylint: disable=redefined-variable-type
if np.abs(dim[1] - np.round(dim[1])) >= 2 * dim_xy * np.finfo(float).eps:
raise ValueError(
"If `dim` is a scalar, it must evenly divide the length of the matrix."
)

All I can really say is that I believe this was an artifact of the way in which the same type of thing was checked in the QETLAB implementation. I'm not sure I have a more satisfying impression of this at the moment. There was a similar check in the realignment.py code as well, as I ended up removing it as I wasn't able to even construct a test case that would hit this block.

Not sure if that's overly zealous or if perhaps there's a reason why this makes sense to do in MATLAB vs. Python. So we could either remove it, or, keep it for now and expect that it will remain uncovered.

@purva-thakre purva-thakre marked this pull request as ready for review December 11, 2023 22:46
@purva-thakre
Copy link
Collaborator Author

marking this ready for review even though the line below is uncovered.

return RuntimeError(
"Unable to determine k-block positivity. Please consider increasing the relative tolerance or the effort level."
)

Copy link
Owner

@vprusso vprusso left a comment

Choose a reason for hiding this comment

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

🚀

@purva-thakre purva-thakre merged commit 223c77c into master Dec 13, 2023
4 checks passed
@purva-thakre purva-thakre deleted the pytest_is_block_positive branch December 13, 2023 23:18
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