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

Features from meuns/galgebra: GA4CS examples, PGA, code-generation, and more #68

Open
wants to merge 87 commits into
base: master
Choose a base branch
from

Conversation

meuns
Copy link
Contributor

@meuns meuns commented Nov 25, 2019

Hi @utensil,

I found a weird thing using pygae/galgebra. Some methods return a Mv instance now but a sympy expression is enough and this crashes Sympy 1.3. Sympy just goes into an infinite recursion using its reflection system and finish by raising an exception.

Regards

meuns added 30 commits March 11, 2016 13:08
test/test_chapter13.py Outdated Show resolved Hide resolved
@eric-wieser

This comment has been minimized.

galgebra/mv.py Outdated Show resolved Hide resolved
@utensil
Copy link
Member

utensil commented Nov 26, 2019

I found a weird thing using pygae/galgebra. Some methods return a Mv instance now but a sympy expression is enough and this crashes Sympy 1.3. Sympy just goes into an infinite recursion using its reflection system and finish by raising an exception.

@meuns Can you make a minimal reproduction of this problem?

galgebra/mv.py Outdated Show resolved Hide resolved
@meuns
Copy link
Contributor Author

meuns commented Nov 30, 2019

I found a weird thing using pygae/galgebra. Some methods return a Mv instance now but a sympy expression is enough and this crashes Sympy 1.3. Sympy just goes into an infinite recursion using its reflection system and finish by raising an exception.

@meuns Can you make a minimal reproduction of this problem?

This is quite easy to reproduce. Just pass any Mv instance to simplify. If I remember well the dot product, the left and right contractions returned some sympy scalar expressions and now they return scalar Mv instances. Not sure this is a problem once the user knows.

@eric-wieser
Copy link
Member

eric-wieser commented Nov 30, 2019

That sounds like the issue I filed the other day then, #53

@utensil

This comment has been minimized.

@utensil
Copy link
Member

utensil commented Dec 1, 2019

If I remember well the dot product, the left and right contractions returned some sympy scalar expressions and now they return scalar Mv instances.

From this description, I can barely recalled that I have done something similar intentionally for consistency. Can't be sure until I get down to try a reprodution.

@eric-wieser
Copy link
Member

eric-wieser commented Dec 16, 2019

If I remember well the dot product, the left and right contractions returned some sympy scalar expressions and now they return scalar Mv instances.

This was done in 6cc530c.

See also #154, which applies this to | too

@@ -700,7 +703,37 @@ def _build_basis_blade_symbol(self, base_index):
raise ValueError('!!!!No unique root symbol for wedge_print = False!!!!')
return Symbol(symbol_str, commutative=False)

def _build_bases(self):
def build_cobases(self, coindexes=None):
Copy link
Member

Choose a reason for hiding this comment

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

xref #215.

I think I understand the rough intent here, although it's not clear to me why a separate set of indices or signs are needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a "canonical" way for building cobases (see John Browne's book) and PGA uses some kind of "tricky" way for building it (J and Jinv stuff). My implementation is probably not the best we can do.

Copy link
Member

Choose a reason for hiding this comment

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

@enkimute, is "cobase" consistent with the terminology you'd use?

Copy link
Contributor Author

@meuns meuns Jan 5, 2020

Choose a reason for hiding this comment

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

The sign is for using the orthogonal wedge implementation provided by galgebra which needs a canonical basis. PGA basis isn't canonical too.

Copy link
Member

Choose a reason for hiding this comment

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

When you say the PGA basis isn't canonical, do you mean that the default is not to use lexicographically-order basis blades? Eg, e132 is considered a "base" blade in that algebra, rather than e123?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap

Copy link
Member

Choose a reason for hiding this comment

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

This forum post captures the issue I assume?

https://discourse.bivector.net/t/dual-operator-disambiguation/59/45

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap

Copy link
Member

Choose a reason for hiding this comment

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

Another way to handle this might just be in the printing only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be very nice if you are able to run test_pga.py without adding the custom indexes and signs. I don't see how we can achieve this.

@eric-wieser eric-wieser changed the title I merged and fixed most of the issues Features from meuns/galgebra: GA4CS examples, PGA, code-generation, and more Apr 30, 2020
Comment on lines +173 to +179
@staticmethod
def _make_blade(ga, __name, __grade, **kwargs):
if isinstance(__name, str) and isinstance(__grade, int):
return reduce(Mv.__xor__, [ga.mv('%s%d' % (__name, i), 'vector') for i in range(__grade)],
ga.mv(1, 'scalar')).obj
else:
raise TypeError("name must be a string and grade must be an int")
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice idea, but unfortunately it won't work with ga.make_grad(some_blade), as the coefficients are no longer single variables.

Perhaps it's worth cherry-picking into master anyway.

Comment on lines +991 to +994
else:
for blade in blade_lst:
if not blade.is_base() or not blade.is_blade():
raise ValueError("%s expression isn't a basis blade" % blade)
Copy link
Member

Choose a reason for hiding this comment

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

This got upstreamed, thanks!

@utensil utensil added this to the 0.6.0 milestone Mar 30, 2024
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