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 interfaces from QEDprocesses.jl #68

Merged
merged 21 commits into from
Jun 24, 2024
Merged

Add interfaces from QEDprocesses.jl #68

merged 21 commits into from
Jun 24, 2024

Conversation

AntonReinhard
Copy link
Member

No description provided.

@AntonReinhard
Copy link
Member Author

Doctests now fail because some of them use functionality that is now in QEDcore. I think the doctests are still useful, is there an easy way to make QEDcore available for the doctests too, similar to the package extensions for the tests?

@AntonReinhard AntonReinhard force-pushed the process_interfaces branch 2 times, most recently from 310edb2 to 62607f2 Compare June 6, 2024 16:27
@AntonReinhard
Copy link
Member Author

Most of the tests have moved to QEDcore for now.

@szabo137
Copy link
Member

szabo137 commented Jun 10, 2024

Doctests now fail because some of them use functionality that is now in QEDcore. I think the doctests are still useful, is there an easy way to make QEDcore available for the doctests too, similar to the package extensions for the tests?

IMHO, the base_state function is not an interface function and can be moved entirely to QEDcore. There, the doctests should work.

I think the same is true for the propagator function.

@AntonReinhard
Copy link
Member Author

Doctests now fail because some of them use functionality that is now in QEDcore. I think the doctests are still useful, is there an easy way to make QEDcore available for the doctests too, similar to the package extensions for the tests?

IMHO, the base_state function is not an interface function and can be moved entirely to QEDcore. There, the doctests should work.

I think the same is true for the propagator function.

I disagree actually. They are useful functions that can be used for multiple purposes and are overloaded on the specific implementations of the ParticleTypes. For example, they're required in the metagraph optimization project, and if we want to make QEDcore replaceable by an alternative implementation (at least in theory), then propagator and base_state should stay in QEDbase as interfaces that need to be implemented.

@AntonReinhard AntonReinhard marked this pull request as ready for review June 13, 2024 08:12
@szabo137 szabo137 self-requested a review June 18, 2024 12:03
@szabo137 szabo137 marked this pull request as draft June 18, 2024 12:04
@AntonReinhard AntonReinhard changed the base branch from dev_refac to dev June 19, 2024 11:42
@szabo137
Copy link
Member

Doctests now fail because some of them use functionality that is now in QEDcore. I think the doctests are still useful, is there an easy way to make QEDcore available for the doctests too, similar to the package extensions for the tests?

IMHO, the base_state function is not an interface function and can be moved entirely to QEDcore. There, the doctests should work.
I think the same is true for the propagator function.

I disagree actually. They are useful functions that can be used for multiple purposes and are overloaded on the specific implementations of the ParticleTypes. For example, they're required in the metagraph optimization project, and if we want to make QEDcore replaceable by an alternative implementation (at least in theory), then propagator and base_state should stay in QEDbase as interfaces that need to be implemented.

Ok, I revoke my statement about base_state and propagator. Since these are needed for every particle anyway, they must be part of the particle interface.

@szabo137
Copy link
Member

szabo137 commented Jun 21, 2024

Seems like the integration test for QEDevents sill breaks, because in there QEDevents.jl is precompiled against the registered version of QEDprocesses.jl and not against QEDprocesses.jl#dev. An obvious solution might be, just releasing QEDprocesses.jl either to general as 0.2.0 or to the local registry (as 0.1.1-DEV). I think the latter is favorable because it does not release and unfinished version of QEDprocesses.jl.

What do you think?

Edit: I opened QEDjl-project/QuantumElectrodynamics.jl#36 to eventually update the integration tests to address these situations. But this is not relevant for this one.

@AntonReinhard
Copy link
Member Author

Seems like the integration test for QEDevents sill breaks, because in there QEDevents.jl is precompiled against the registered version of QEDprocesses.jl and not against QEDprocesses.jl#dev. An obvious solution might be, just releasing QEDprocesses.jl either to general as 0.2.0 or to the local registry (as 0.1.1-DEV). I think the latter is favorable because it does not release and unfinished version of QEDprocesses.jl.

What do you think?

Yes sounds good. I wouldn't release publicly at this state, so local registry seems like the way to go.

@AntonReinhard AntonReinhard marked this pull request as ready for review June 21, 2024 19:53
@AntonReinhard
Copy link
Member Author

AntonReinhard commented Jun 21, 2024

Testing QEDjl-project/QEDcore.jl#25 with this branch works for me locally, so the two should be good to merge together.

szabo137
szabo137 previously approved these changes Jun 21, 2024
Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

Very good work! Thanks for the movement. We need to refactor the tests of the interfaces at some point. Maybe another issue to be opened 😅

docs/make.jl Show resolved Hide resolved
src/QEDbase.jl Outdated Show resolved Hide resolved
src/interfaces/particle.jl Outdated Show resolved Hide resolved
src/interfaces/phase_space_point.jl Outdated Show resolved Hide resolved
src/interfaces/process.jl Outdated Show resolved Hide resolved
src/particles/spin_pol.jl Outdated Show resolved Hide resolved
src/particles/spin_pol.jl Outdated Show resolved Hide resolved
src/particles/spin_pol.jl Outdated Show resolved Hide resolved
src/particles/spin_pol.jl Outdated Show resolved Hide resolved
src/utils.jl Show resolved Hide resolved
@szabo137 szabo137 added the 09 - Maintenance Related to maintenance, housekeeping, repo-config label Jun 21, 2024
Co-authored-by: Uwe Hernandez Acosta <szabo137@users.noreply.github.com>
Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

All conversations are resolved. No concerns from my side. Let the merge begin!

@szabo137 szabo137 merged commit 42adfb2 into dev Jun 24, 2024
3 checks passed
@szabo137 szabo137 added this to the Release-next milestone Jun 24, 2024
@AntonReinhard AntonReinhard deleted the process_interfaces branch June 24, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
09 - Maintenance Related to maintenance, housekeeping, repo-config
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants