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

reverting real coordinates #163

Merged
merged 2 commits into from
Jul 24, 2024
Merged

reverting real coordinates #163

merged 2 commits into from
Jul 24, 2024

Conversation

campospinto
Copy link
Collaborator

The PR #156 defined domain coordinates with real=True argument. This is probably justified but requires additional testing as it breaks some calls to TerminalExpr. Here this change is reverted, and a dedicated PR will be reopened later.

@campospinto campospinto self-assigned this Jul 24, 2024
Copy link
Contributor

@e-moral-sanchez e-moral-sanchez left a comment

Choose a reason for hiding this comment

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

Ok. I will take a look at this later on. Thanks!

@campospinto campospinto merged commit 73a5998 into master Jul 24, 2024
6 checks passed
@campospinto campospinto deleted the revert_real_coordinates branch July 24, 2024 16:42
@yguclu
Copy link
Member

yguclu commented Aug 1, 2024

@campospinto @e-moral-sanchez What is the current status on this matter? Is there an open issue?

@campospinto
Copy link
Collaborator Author

campospinto commented Aug 2, 2024

@campospinto @e-moral-sanchez What is the current status on this matter? Is there an open issue?

I don't think so, but it would be good indeed to have one that describes precisely the problem.

Note that the previous PR #156 was breaking some calls to TerminalExpr of derivatives (returning zeros for non-constant functions), probably because the coordinate symbols were not properly identified. Some of the corresponding code is now tested in the (still open) PR #161, so that the problem can be detected at sympde level.

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