Skip to content

Remove assignments to mv.obj#141

Merged
utensil merged 3 commits intopygae:masterfrom
eric-wieser:remove-obj-assignments
Dec 8, 2019
Merged

Remove assignments to mv.obj#141
utensil merged 3 commits intopygae:masterfrom
eric-wieser:remove-obj-assignments

Conversation

@eric-wieser
Copy link
Member

It is much easier to reason about multivector objects if they are are not mutated unexpectedly.

This changes the following functions to not mutate their arguments. Any code relying solely on the return value will be unaffected:

  • Metric.applymv
  • Mv.diff
  • Mv.simplify
  • Mv.expand
  • Mv.collect

This also changes Mv.Mv_str and Mv.Mv_latex_str to not mutate their multivectors when they are printed - this is super surprising behavior

cc @meuns

It is much easier to reason about multivector objects if they are are not mutated unexpectedly.

This changes the following functions to not mutate their arguments. Any code relying solely on the return value will be unaffected:

* `Metric.applymv`
* `Mv.diff`
* `Mv.simplify`
* `Mv.expand`
* `Mv.collect`

This also changes `Mv.Mv_str` and `Mv.Mv_latex_str` to not mutate their multivectors when they are printed - this is super surprising behavior
This was a bad workaround to pass variables to a closure in the printer
@eric-wieser eric-wieser force-pushed the remove-obj-assignments branch from 6ce004b to 588c016 Compare December 7, 2019 18:13
@lgtm-com
Copy link

lgtm-com bot commented Dec 7, 2019

This pull request introduces 1 alert when merging 588c016 into 159cc2a - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@eric-wieser
Copy link
Member Author

eric-wieser commented Dec 7, 2019

@utensil, CI seems to be producing unhelpful output in https://travis-ci.com/pygae/galgebra/jobs/264351319. Any idea how to debug?

(edit: found the bug, but working out how to make CI more useful might still be worth it)

@pygae pygae deleted a comment from lgtm-com bot Dec 7, 2019
@eric-wieser eric-wieser marked this pull request as ready for review December 7, 2019 19:34
@eric-wieser eric-wieser requested a review from utensil December 7, 2019 19:34
@lgtm-com
Copy link

lgtm-com bot commented Dec 7, 2019

This pull request introduces 1 alert when merging 094e06f into 159cc2a - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@utensil
Copy link
Member

utensil commented Dec 7, 2019

@utensil, CI seems to be producing unhelpful output in https://travis-ci.com/pygae/galgebra/jobs/264351319. Any idea how to debug?

(edit: found the bug, but working out how to make CI more useful might still be worth it)

This pattern of output log suggests that:

  1. Something went wrong during the execution of !python physics_check_latex.py;
  2. Thus !cat physics_check_latex.tex will see no physics_check_latex.tex and fail as well.

1 is not outputing useful info due to the mechanism in nbval. For the same output field, it will output the diff, but if the execution caused the output field changed from stdout to stderr, for example, then it will not output the expected field and the actual field.

The easiest way to debug it is to run 1 by ourselves and see what it outputs.

Expecting more requires contributing to nbval.

Travis CI enables your team to test and ship your apps with confidence. Easily sync your projects with Travis CI and you'll be testing your code in minutes.

@eric-wieser
Copy link
Member Author

I'll take a look at nbval at some point, thanks for the summary.

Copy link
Member

@utensil utensil left a comment

Choose a reason for hiding this comment

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

Neat!

@utensil utensil merged commit 8e21d5b into pygae:master Dec 8, 2019
@eric-wieser
Copy link
Member Author

Guess I'll fix the LGTM warning in a follow-up - I supposed the cost of the warning is outweighed by the reduced surprise for @meuns :)

eric-wieser added a commit to eric-wieser/galgebra that referenced this pull request Dec 8, 2019
Introduced in pygaegh-141, which removed all uses of this variable but forgot to remove it.
eric-wieser added a commit that referenced this pull request Dec 8, 2019
Introduced in gh-141, which removed all uses of this variable but forgot to remove it.
@eric-wieser eric-wieser deleted the remove-obj-assignments branch December 8, 2019 10:56
@utensil utensil added this to the 0.4.5 milestone Dec 18, 2019
@eric-wieser eric-wieser added the component: core Ga, Mv, Metric, etc label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: core Ga, Mv, Metric, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants