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

[ITensors] Observers.jl package extension #1381

Merged
merged 20 commits into from
Apr 16, 2024
Merged

Conversation

emstoudenmire
Copy link
Collaborator

This PR is work in progress toward adding Observers support as a package extension. I am going to gradually submit commits and re-test to pinpoint (or avoid) the testing issue with previous PRs.

Project.toml Outdated Show resolved Hide resolved
@mtfishman mtfishman changed the title Add weakdep and compat for Observers [ITensors[ Add weakdep and compat for Observers Apr 4, 2024
@mtfishman mtfishman changed the title [ITensors[ Add weakdep and compat for Observers [ITensors] Add weakdep and compat for Observers Apr 4, 2024
@mtfishman
Copy link
Member

@emstoudenmire in #1365 @kmp5VT is trying out running the tests in a temporary project with Pkg.activate(temp=true), I wonder if that will help here since my guess is that it is using an old poorly configured Manifest.toml instead of building one from scratch.

@mtfishman
Copy link
Member

@emstoudenmire seems like the updated CI workflow fixed the issue.

@emstoudenmire
Copy link
Collaborator Author

That's a relief! Thanks to you and Karl for figuring out that fix.

@emstoudenmire
Copy link
Collaborator Author

Still some more changes to make on this PR, but I think only some small ones left.

@emstoudenmire
Copy link
Collaborator Author

Unles you prefer to just make this PR about removing Observers. Then I could submit a new one for the Observers extension. Otherwise I'll just change the PR name and add those commits here.

@mtfishman
Copy link
Member

Let's make the package extension in this PR.

@mtfishman mtfishman changed the title [ITensors] Add weakdep and compat for Observers [ITensors] Observers.jl package extension Apr 12, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 12, 2024

Codecov Report

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

Project coverage is 79.55%. Comparing base (e8197ce) to head (3bc1013).
Report is 7 commits behind head on main.

❗ Current head 3bc1013 differs from pull request most recent head 7e349eb. Consider uploading reports for the commit 7e349eb to get more accurate results

Files Patch % Lines
src/ITensorMPS/alternating_update.jl 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1381      +/-   ##
==========================================
- Coverage   79.59%   79.55%   -0.05%     
==========================================
  Files         114      114              
  Lines        9032     9032              
==========================================
- Hits         7189     7185       -4     
- Misses       1843     1847       +4     

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

@emstoudenmire
Copy link
Collaborator Author

[test ITensorTDVP]

Copy link
Contributor

Run ITensorTDVP tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8668387751

@emstoudenmire
Copy link
Collaborator Author

This is running the ITensorTDVP workflow on main, as I guessed it would (though the tests ran and passed, for what it's worth, though I think not against this PR). So I'm not sure if the new one I included in this PR will work better or work correctly. If you have a good suggestion for how to repeatedly test a workflow file in a PR I'd be curious. Or maybe another way is to push

Copy link
Contributor

Run ITensorTDVP tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/8668675788

@emstoudenmire emstoudenmire marked this pull request as ready for review April 13, 2024 16:37
@emstoudenmire
Copy link
Collaborator Author

emstoudenmire commented Apr 13, 2024

I'm pretty sure the new ITensorTDVP test is correctly running the code from this PR. There is the line:
[9136182c] ITensors v0.3.67 `~/work/ITensors.jl/ITensors.jl`
showing up in the output (under "Install Julia Dependencies and Run Tests") from Github Actions. Also I based it closely on the ITensors Base tests workflow.

If we want to be totally sure, though, I could add a printing statement (like println("PR 1381")) to e.g. the tdvp function inside ITensorMPS. Then we should see the tests pass but with that printing statement running a bunch of times and showing up in the test output. So then we could know for sure that it's running using the code from this PR.

(Update: I'm trying this now.)

@emstoudenmire
Copy link
Collaborator Author

I tried my idea about the printing statement, and re-ran the ITensorTDVP test. Unfortunately, though, I did not see the printing output when the tests ran. (I had put a simple println statement at the top of the itensortdvp_tdvp function, a version of it that I'm pretty sure gets covered by the tests.

@mtfishman
Copy link
Member

mtfishman commented Apr 13, 2024

I tried my idea about the printing statement, and re-ran the ITensorTDVP test. Unfortunately, though, I did not see the printing output when the tests ran. (I had put a simple println statement at the top of the itensortdvp_tdvp function, a version of it that I'm pretty sure gets covered by the tests.

The ITensorTDVP testing workflow is running the tests of the latest registered version of ITensorTDVP.jl, which has its own implementation of tdvp which isn't calling ITensorMPS.itensortdvp_tdvp, since ITensor/ITensorTDVP.jl#67 isn't merged and registered yet. So I think the current behavior is expected.

We could put a print statement in some other part of the code, say inside one of the ProjMPO functions that is being used by the current ITensorTDVP.tdvp function.

@emstoudenmire
Copy link
Collaborator Author

That makes sense, thanks. I'll move the printing statement.

@mtfishman
Copy link
Member

Putting it in the makeR! of ProjMPOApply also doesn't work since ITensorTDVP.jl currently has its own ProjMPOApply that it is using: https://github.com/ITensor/ITensorTDVP.jl/blob/v0.2.1/src/projmpo_apply.jl.

@emstoudenmire
Copy link
Collaborator Author

I temporarily moved the printing statement to the norm function in abstractmps.jl and finally saw it in the TDVP testing output. So we can be sure that it's testing from the current PR now. I've since removed that printing statement altogether and now re-running all the tests.

I believe this is ready to merge once tests pass.

@mtfishman mtfishman merged commit 4d64299 into main Apr 16, 2024
18 of 19 checks passed
@mtfishman mtfishman deleted the observers_extension4 branch April 16, 2024 11:35
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.

3 participants