-
Notifications
You must be signed in to change notification settings - Fork 3
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
Remove abstract cs and probability implementations #87
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fast PR! Looks good for me. Will merge if the pipeline succeeds.
Edit: Oh, the tests, you are right 😅
Yes, almost forgot 😅 Should be good if the pipeline goes through 👍 Edit: Right, it won't succeed. It now needs the implementations from QEDbase. I think we need to target this branch in the QEDbase integration tests, then merge the QEDbase PR, then merge this one. |
Could you please add CI_UNIT_PKG_URL_QEDbase: https://github.com/szabo137/QEDbase.jl#2492e986972de30fadd68cd7fa7946dc4d888a61 to your last commit message? Then the unit tests should succeed. If so, we can tell QEDjl-project/QEDbase.jl#87 to use this fix here and merge it. Afterwards we can merge here. |
81ccd3a
to
c34ef52
Compare
Something seems to break with the pipeline. It finds and clones the correct QEDbase version, but somehow there seems to be a git merge happening in the Project.toml, leading to a parsing error |
My bad, wrong commit hash 🤦♂️ |
c34ef52
to
2394818
Compare
Please remove the CI_UNIT_PKG_URL_QEDbase from the last commit message and force push again.. this should use the current |
2394818
to
cea8e0c
Compare
The implementations of the cross sections and probability functions for abstract processes have moved to QEDbase, see QEDjl-project/QEDbase.jl#87. They are therefore now deleted here.
Fixes issue QEDjl-project/QEDbase.jl#86