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

Remove PSDEF in favor of PSL #150

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

AntonReinhard
Copy link
Member

The docs building still fails, because unfortunately the documentation still has a cyclic dependency to QEDcore and QEDprocesses. I think the best way forward is to remove the dependency from the literate.jl tutorials by using self-defined custom types and removing or changing the jldoctests that rely on QEDcore or processes. Those should probably be moved to their respective packages.

@AntonReinhard
Copy link
Member Author

The docs should build for now, but they're not very pretty yet. They now use each other's custom definitions instead of QEDcore or QEDprocesses. For this I had to add a FourMomentum and a AbstractModel tutorial, both of which are very bare bones for the moment.

I also had to disable some of the jldoctests where I'm not sure if they're doable nicely here. I just changed them to julia codeblocks instead of jldoctest for now. I think the functionality jldoctests should generally be moved to where it's implemented, i.e. QEDcore or QEDprocesses.

Other than that I think this is the way to go, because otherwise our cyclic dependencies problem will still persist.

@szabo137 thoughts?

@szabo137
Copy link
Member

The docs should build for now, but they're not very pretty yet. They now use each other's custom definitions instead of QEDcore or QEDprocesses. For this I had to add a FourMomentum and a AbstractModel tutorial, both of which are very bare bones for the moment.

I also had to disable some of the jldoctests where I'm not sure if they're doable nicely here. I just changed them to julia codeblocks instead of jldoctest for now. I think the functionality jldoctests should generally be moved to where it's implemented, i.e. QEDcore or QEDprocesses.

Other than that I think this is the way to go, because otherwise our cyclic dependencies problem will still persist.

@szabo137 thoughts?

I think for now, it is fine to leave it like that. Eventually, we will have TestImplementation ready to be a dependency of the docs (see #151 gor details). Then we could just use it to mock the functionality from QEDcore, which is not available anymore.

@AntonReinhard
Copy link
Member Author

Okay sounds good, then I'll just go over it a little bit tomorrow and it should be ready. Then we'll have to fix the integration tests by adjusting QEDcore.

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.

In general it looks very good, many thanks for that!
I've just some comments and one request: please look up the PSL interface and use it properly in the construction of the PSPs.

docs/make.jl Show resolved Hide resolved
docs/src/tutorial/phase_space_point.jl Show resolved Hide resolved
test/gpu/runtests.jl Outdated Show resolved Hide resolved
src/implementations/process/momenta.jl Outdated Show resolved Hide resolved
@szabo137
Copy link
Member

Okay sounds good, then I'll just go over it a little bit tomorrow and it should be ready. Then we'll have to fix the integration tests by adjusting QEDcore.

This is correct! I think after adjusting QEDcore, the other packages are pretty easy to fix.

I think it is our first multi-level fix, showing the greatness of our integration test suite 👯‍♂️ ( @SimeonEhrig stay tuned if we break something, again 🫶)

@AntonReinhard
Copy link
Member Author

please look up the PSL interface and use it properly in the construction of the PSPs.

You're completely right. I mostly just removed the psdef and replaced occurrences with the psl, intending to see what needs to be changed when it breaks. But it seems it didn't break in those instances so I forgot to actually use the new interface ^^

@szabo137
Copy link
Member

please look up the PSL interface and use it properly in the construction of the PSPs.

You're completely right. I mostly just removed the psdef and replaced occurrences with the psl, intending to see what needs to be changed when it breaks. But it seems it didn't break in those instances so I forgot to actually use the new interface ^^

This was also my guess ^^
However, I don't know, maybe this is the time to add some unit tests for the PSPs to check the construction from coordinates — TDD style.

@SimeonEhrig
Copy link
Member

I think it is our first multi-level fix, showing the greatness of our integration test suite 👯‍♂️ ( @SimeonEhrig stay tuned if we break something, again 🫶)

Be aware, my weekends starts 5 minutes before you break my CI ^^

@szabo137 szabo137 self-requested a review January 23, 2025 18:29
szabo137
szabo137 previously approved these changes Jan 23, 2025
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.

From my side, this looks good now. Therefore, we can proceed fixing the integration tests by opening the respective PRs in the other packages.

@szabo137
Copy link
Member

@AntonReinhard I removed the PSDEF from QEDcore: QEDjl-project/QEDcore.jl#94

Please retrigger the CI with this fix. Locally, the unittests in QEDjl-project/QEDcore.jl#94 pass with this version of QEDbase.

@AntonReinhard
Copy link
Member Author

The formatter fails on the tutorials, because I did not indent the includes, which would have to be indented, but the reason for the indent is not visible in the docs, so in the docs it looks weird. And if I turn off the format for those lines with a format: off, then that comment is also visible in the docs. Do we just want to turn off the format check for the tutorials?

@AntonReinhard AntonReinhard force-pushed the remove-psdef branch 2 times, most recently from f74265d to 136dc60 Compare January 29, 2025 13:15
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