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

Add pratio and maxdim #89

Merged
merged 8 commits into from
Jun 3, 2022
Merged

Add pratio and maxdim #89

merged 8 commits into from
Jun 3, 2022

Conversation

eliascarv
Copy link
Member

@eliascarv eliascarv commented May 31, 2022

This PR adds pratio option to EigenAnalysis and change ndim to maxdim.
I made some decisions to this implementation:

  • Make pratio and maxdim kwargs to give the user option to choose one of these, both or neither.
  • Change ndim to maxdim to make sense with pratio implementation.

closes #88

@eliascarv eliascarv requested a review from juliohm May 31, 2022 14:41
@eliascarv eliascarv changed the title Add pratio and maxdim WIP: Add pratio and maxdim May 31, 2022
Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

Hi @eliascarv I can give a more detailed review after 17hs BRT today. I wonder if we should preserve the semantic of ndim and use ndim and pratio as mutually exclusive options. That is what I had in mind originally.

Let's chat over Zulip to align detail later today.

src/transforms/eigenanalysis.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #89 (fed417c) into master (0db52f0) will increase coverage by 0.50%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   93.12%   93.62%   +0.50%     
==========================================
  Files          23       24       +1     
  Lines         567      596      +29     
==========================================
+ Hits          528      558      +30     
+ Misses         39       38       -1     
Impacted Files Coverage Δ
src/transforms/eigenanalysis.jl 83.58% <95.23%> (+3.92%) ⬆️
src/transforms/sort.jl 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0db52f0...fed417c. Read the comment docs.

@eliascarv eliascarv marked this pull request as ready for review June 3, 2022 18:33
@eliascarv eliascarv requested a review from juliohm June 3, 2022 18:33
@eliascarv eliascarv changed the title WIP: Add pratio and maxdim Add pratio and maxdim Jun 3, 2022
Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

Review attached.

src/transforms/eigenanalysis.jl Outdated Show resolved Hide resolved
src/transforms/eigenanalysis.jl Outdated Show resolved Hide resolved
@juliohm
Copy link
Member

juliohm commented Jun 3, 2022

Amazing PR as usual @eliascarv , super clean and professional coding 💯 Let's just wait for the tests to pass.

@juliohm juliohm merged commit b6e1ee7 into master Jun 3, 2022
@juliohm juliohm deleted the pratio branch June 3, 2022 19:00
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.

Add pratio option to EigenAnalysis
3 participants