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

Small refactoring for hermitian check #3296

Merged
merged 1 commit into from Mar 23, 2023
Merged

Small refactoring for hermitian check #3296

merged 1 commit into from Mar 23, 2023

Conversation

blegat
Copy link
Member

@blegat blegat commented Mar 22, 2023

Less duplicate code is better

@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (f4721e6) 98.12% compared to head (818c957) 98.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3296      +/-   ##
==========================================
- Coverage   98.12%   98.12%   -0.01%     
==========================================
  Files          34       34              
  Lines        4740     4738       -2     
==========================================
- Hits         4651     4649       -2     
  Misses         89       89              
Impacted Files Coverage Δ
src/sd.jl 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@odow
Copy link
Member

odow commented Mar 22, 2023

Less duplicate code is better

I don't agree. It moves the error into a utility function, when it should really be in build_variable. I think that makes it less readable. It's not obvious that _vectorize_complex_variables should throw an error if one of them is discrete.

@blegat
Copy link
Member Author

blegat commented Mar 23, 2023

If you have discrete constraints then in the vectorization itself, you should have to handle them, e.g., decide the value of the imag part of a boolean variable. Since the utility function does not handle that yet, it makes sense that it errors in that case.

@odow odow merged commit fd1b2dc into master Mar 23, 2023
12 checks passed
@odow odow deleted the bl/_vec_comp_int branch March 23, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants