Skip to content

Add rejection sampler#70

Draft
szabo137 wants to merge 6 commits into
QEDjl-project:devfrom
szabo137:add-rejection-sampler
Draft

Add rejection sampler#70
szabo137 wants to merge 6 commits into
QEDjl-project:devfrom
szabo137:add-rejection-sampler

Conversation

@szabo137
Copy link
Copy Markdown
Member

@szabo137 szabo137 commented Oct 22, 2025

  • add backend-specific test suite
  • Implement HardScatteringTarget
  • write test suite for HardScatteringTarget

@szabo137 szabo137 force-pushed the add-rejection-sampler branch 3 times, most recently from ff14750 to 8e91f1d Compare October 22, 2025 14:50
AntonReinhard
AntonReinhard previously approved these changes Oct 22, 2025
Copy link
Copy Markdown
Member

@AntonReinhard AntonReinhard left a comment

Choose a reason for hiding this comment

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

Looks good

@szabo137
Copy link
Copy Markdown
Member Author

Currently, test fail due to RejectionSamplers not being registered.

Comment thread test/runtests.jl
@AntonReinhard
Copy link
Copy Markdown
Member

This should (hopefully) get fixed once QEDjl-project/QEDprocesses.jl#141 is merged

Comment thread src/testutils/TestUtils.jl
@AntonReinhard
Copy link
Copy Markdown
Member

I tested it locally before, it really should be working now. Maybe a push is necessary to update the CI?

@AntonReinhard
Copy link
Copy Markdown
Member

I tested it locally before, it really should be working now. Maybe a push is necessary to update the CI?

Oh, it's not working because QEDprocesses is only a dependency in extras, so the CI doesn't load the dev version, only the released version of it, which doesn't have the fix

@szabo137
Copy link
Copy Markdown
Member Author

szabo137 commented Nov 2, 2025

I tested it locally before, it really should be working now. Maybe a push is necessary to update the CI?

Oh, it's not working because QEDprocesses is only a dependency in extras, so the CI doesn't load the dev version, only the released version of it, which doesn't have the fix

Indeed, it is still not working. And the error message is weird because it refers to SVector{16, Complex}, which should not be present anymore if the correct dev branch of QEDprocesses were used. 🤷‍♂️

@AntonReinhard
Copy link
Copy Markdown
Member

Indeed, it is still not working. And the error message is weird because it refers to SVector{16, Complex}, which should not be present anymore if the correct dev branch of QEDprocesses were used. 🤷‍♂️

Yeah, as I said, it's not using the dev version because QEDprocesses is not a "real" dependency, only an [extra] dependency.

@AntonReinhard
Copy link
Copy Markdown
Member

I wanted to release patch versions for base and processes anyway, which should fix this. Also I believe we decided that processes is going to become a real dependency at some point, so you could just add it here too.

@szabo137
Copy link
Copy Markdown
Member Author

szabo137 commented Nov 2, 2025

Indeed, it is still not working. And the error message is weird because it refers to SVector{16, Complex}, which should not be present anymore if the correct dev branch of QEDprocesses were used. 🤷‍♂️

Yeah, as I said, it's not using the dev version because QEDprocesses is not a "real" dependency, only an [extra] dependency.

Understood (I missed your previous answer for some reason 🤷‍♂️). So it should work if we release QEDprocesses (and QEDbase)? I try to think about a simple way to add the extensions manually, without digging too deep into IntegrationTests.jl (again, sorry @SimeonEhrig 😅)

@szabo137 szabo137 marked this pull request as draft November 2, 2025 23:03
@szabo137 szabo137 force-pushed the add-rejection-sampler branch from e793594 to 4ade8bd Compare November 3, 2025 12:35
@szabo137 szabo137 force-pushed the add-rejection-sampler branch from 4ade8bd to 1d13215 Compare November 3, 2025 13:54
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