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 missing kwargs... in evolve-related functions #267

Merged
merged 6 commits into from
Dec 4, 2024

Conversation

jofrevalles
Copy link
Member

This PR includes changes to evolve-related functions to add support for additional keyword arguments (kwargs...) in several functions. This enhancement allows for more flexible function calls by passing extra parameters as needed.

This is useful for example when we want to pass a kwarg into truncate.

@jofrevalles jofrevalles requested a review from mofeing December 3, 2024 11:17
Copy link
Member

@mofeing mofeing left a comment

Choose a reason for hiding this comment

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

I totally prefer using kwargs... more often, but keep in mind that due to its nature it can affect many levels of calls of methods.

The changes I propose in the review are mainly the following:

  • If threshold, maxdim and normalize are not directly used, but just passed, then remove them and let's pass them as part of kwargs.
  • If you just need to know if threshold and maxdim are defined (i.e. the are not nothing), then use !isnothing(get(kwargs, :threshold, nothing))
    • What do you think about moving this check to truncate!? like make threshold=nothing and maxdim=nothing by default in truncate! and do the check also there. If I recall correctly, we don't do the check there and that's why we must do it here. I think it will look cleaner that way.
  • If you need to override values from kwargs and ensure some kwarg configuration, then call do kwargs... first and then do the override.

@jofrevalles
Copy link
Member Author

  • What do you think about moving this check to truncate!? like make threshold=nothing and maxdim=nothing by default in truncate! and do the check also there. If I recall correctly, we don't do the check there and that's why we must do it here. I think it will look cleaner that way

I agree, this makes sense! I will do that.

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes missing coverage. Please review.

Project coverage is 0.04%. Comparing base (0ba1707) to head (c4d3c2f).
Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
src/MPS.jl 0.00% 20 Missing ⚠️
src/Ansatz.jl 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #267       +/-   ##
==========================================
- Coverage   79.59%   0.04%   -79.55%     
==========================================
  Files          32      32               
  Lines        2068    2150       +82     
==========================================
- Hits         1646       1     -1645     
- Misses        422    2149     +1727     

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

@jofrevalles jofrevalles requested a review from mofeing December 4, 2024 11:07
@jofrevalles
Copy link
Member Author

Okay @mofeing let's merge this

Copy link
Member

@mofeing mofeing left a comment

Choose a reason for hiding this comment

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

super, just some lil suggestions and we're done

src/Ansatz.jl Outdated
Comment on lines 271 to 274
return tn
else
return truncate!(form(tn), tn, bond; threshold, maxdim, kwargs...)
end
Copy link
Member

Choose a reason for hiding this comment

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

just an aesthetic recommendation

Suggested change
return tn
else
return truncate!(form(tn), tn, bond; threshold, maxdim, kwargs...)
end
return tn
end
return truncate!(form(tn), tn, bond; threshold, maxdim, kwargs...)

src/MPS.jl Outdated
Comment on lines 585 to 586
if any(!isnothing, get.(Ref(kwargs), [:threshold, :maxdim], nothing))
truncate_sweep!(form(ψ), ψ; kwargs...)
Copy link
Member

Choose a reason for hiding this comment

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

probably better if you move this check to truncate_sweep! as you've done with truncate!

Copy link
Member Author

Choose a reason for hiding this comment

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

Well okay, but I still need to check that is some way to normalize if it is not truncated.

src/MPS.jl Outdated
initial_form = form(ψ)
mixed_canonize!(ψ, Site(nsites(ψ))) # We convert all the tensors to left-canonical form

normalize = get(kwargs, :normalize, false)
Copy link
Member

Choose a reason for hiding this comment

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

here the default is false but NonCanonical is true?
we must keep consistent

Copy link
Member Author

Choose a reason for hiding this comment

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

Well okay, but this does not change anything, since we send normalize with default=true in evolve(ψ::AbstractAnsatz, mpo::AbstractMPO; reset_index=true, kwargs...).

src/MPS.jl Outdated
Comment on lines 616 to 617
if any(!isnothing, get.(Ref(kwargs), [:threshold, :maxdim], nothing))
truncate_sweep!(Canonical(), ψ; kwargs...)
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@jofrevalles jofrevalles merged commit c7dbf25 into master Dec 4, 2024
5 checks passed
@jofrevalles jofrevalles deleted the fix/evolve_kwargs branch December 4, 2024 11:57
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