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.

Sign up for GitHub

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

Jump to bottom

Phase space layout interface #134

Merged
merged 6 commits into from
Nov 21, 2024
Merged

Phase space layout interface #134

merged 6 commits into from
Nov 21, 2024

Conversation

szabo137
Copy link
Member

@szabo137 szabo137 commented Oct 29, 2024 •
edited
Loading

Adds the phase space layout (PSL) interface

TODOs

  • consider merging after QEDprocesses=0.3 is released

Edit:
The remaining todos are moved to #140

@szabo137 szabo137 force-pushed the phase_space_layout_interface branch from 6f28ebd to 9e905ff Compare October 31, 2024 09:23
This was referenced Oct 31, 2024
Merged
Closed
@SimeonEhrig SimeonEhrig force-pushed the phase_space_layout_interface branch from 4b87e7e to 9e905ff Compare November 4, 2024 09:18
@SimeonEhrig
Copy link
Member

@szabo137 Sorry for the confusion. VSC likes to push on the wrong remote 🙄

@szabo137 szabo137 marked this pull request as draft November 4, 2024 10:31
This was referenced Nov 4, 2024
Closed
Merged
@szabo137
Copy link
Member Author

szabo137 commented Nov 4, 2024

The fix for the unit tests are given here: QEDjl-project/QEDcore.jl#84

@SimeonEhrig SimeonEhrig force-pushed the phase_space_layout_interface branch from b0d61e8 to 9e905ff Compare November 4, 2024 16:06
AntonReinhard
AntonReinhard reviewed Nov 5, 2024
View reviewed changes
Copy link
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.

The structure and design looks good to me, as we have already talked about. I mostly have a bunch of typos and some documentation suggestions.

src/implementations/phase_space_layout/build_momenta.jl Outdated Show resolved Hide resolved
src/implementations/phase_space_layout/build_momenta.jl Outdated Show resolved Hide resolved
src/implementations/phase_space_layout/build_momenta.jl Outdated Show resolved Hide resolved
src/implementations/phase_space_layout/build_momenta.jl Outdated Show resolved Hide resolved
src/interfaces/phase_space_layout.jl Outdated Show resolved Hide resolved
src/interfaces/phase_space_layout.jl Outdated Show resolved Hide resolved
src/interfaces/phase_space_layout.jl Outdated Show resolved Hide resolved
src/interfaces/phase_space_layout.jl Outdated Show resolved Hide resolved
src/QEDbase.jl Outdated Show resolved Hide resolved
test/interfaces/phase_space_layout.jl Outdated Show resolved Hide resolved
@AntonReinhard
Copy link
Member

The fix for the unit tests are given here: QEDjl-project/QEDcore.jl#84

I think you'll need to link the PRs via the commit messages so the tests pass and we can merge.

@szabo137 szabo137 force-pushed the phase_space_layout_interface branch from 9e905ff to 1b607e9 Compare November 12, 2024 13:57
@AntonReinhard AntonReinhard linked an issue Nov 14, 2024 that may be closed by this pull request
Refactoring: Phase space definition #139
Closed
@szabo137 szabo137 force-pushed the phase_space_layout_interface branch from 65e6c8c to 5fbf3ad Compare November 14, 2024 20:24
@szabo137
Copy link
Member Author

@SimeonEhrig Sorry for my dumb question, again. As you can see in the test pipelines, this one's unit and integration tests work with a specific branch of QEDcore. Since this is not the dev branch, the verification job fails, which is intended, if I understand correctly. But now my dumb question: how should we proceed from here? I mean, this one can not be merged due to the verification failure, but QEDjl-project/QEDcore.jl#84 can't be merged either, because it needs this PR to be merged.

@AntonReinhard
Copy link
Member

I believe it can be merged only if you use the CI_INTG_PKG_URL_QEDcore comment, not with the UNIT one. The unit tests must succeed without a different branch, because otherwise they would fail on dev after a merge.

@szabo137
Copy link
Member Author

szabo137 commented Nov 16, 2024 •
edited
Loading

This I understood, so that means, I need to hide the PhaseSpaceLayout interface behind the QEDbase namespace to make its unit tests pass with QEDcore#dev and then proceed from there?

@AntonReinhard
Copy link
Member

This I understood, so that means, I need to hide the PhaseSpaceLayout interface behind the QEDbase namespace to make its unit tests pass with QEDcore#dev and then proceed from there?

Yes, I think then it should work.

@szabo137 szabo137 marked this pull request as ready for review November 21, 2024 16:40
@szabo137 szabo137 merged commit 9a7f084 into QEDjl-project:dev Nov 21, 2024
4 checks passed
AntonReinhard pushed a commit to QEDjl-project/QEDcore.jl that referenced this pull request Nov 21, 2024
@szabo137
removed patch QEDbase (#84)
8a9976a 
- [x] consider for merging after
QEDjl-project/QEDbase.jl#134 is merged

---------

Co-authored-by: Uwe Hernandez Acosta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AntonReinhard AntonReinhard AntonReinhard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactoring: Phase space definition
3 participants
@szabo137 @SimeonEhrig @AntonReinhard