Skip to content

Commit b4ff61b

Browse files
author
Kasper Peeters
committed
Fix sort_product bug which kept certain expressions unsorted.
1 parent 126a8b7 commit b4ff61b

File tree

2 files changed

+62
-37
lines changed

2 files changed

+62
-37
lines changed

‎core/Compare.cc‎

Lines changed: 51 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,14 @@
1717
#include "properties/Integer.hh"
1818
#include "properties/SortOrder.hh"
1919

20-
// In order to enable/disable debug output, also flip the swith in 'report' below.
21-
// #define DEBUG(ln) ln
22-
#define DEBUG(ln)
20+
//#define DEBUG 1
21+
22+
23+
#ifdef DEBUG
24+
#define DEBUGLN(ln) ln
25+
#else
26+
#define DEBUGLN(ln)
27+
#endif
2328

2429
namespace cadabra {
2530

@@ -60,7 +65,7 @@ namespace cadabra {
6065
else return -mult;
6166
}
6267

63-
DEBUG( std::cerr << "auto? " << one->is_autodeclare_wildcard() << ", " << two->is_numbered_symbol() << std::endl; );
68+
DEBUGLN( std::cerr << "auto? " << one->is_autodeclare_wildcard() << ", " << two->is_numbered_symbol() << std::endl; );
6469
if( (one->is_autodeclare_wildcard() && two->is_numbered_symbol()) ||
6570
(two->is_autodeclare_wildcard() && one->is_numbered_symbol()) ) {
6671
if( one->name_only() != two->name_only() ) {
@@ -73,7 +78,7 @@ namespace cadabra {
7378
else return -mult;
7479
}
7580

76-
DEBUG( std::cerr << "match despite difference: " << *one->name << " and " << *two->name << std::endl; );
81+
DEBUGLN( std::cerr << "match despite difference: " << *one->name << " and " << *two->name << std::endl; );
7782
}
7883

7984
// Compare parent relations.
@@ -299,7 +304,7 @@ namespace cadabra {
299304
Ex_comparator::match_t Ex_comparator::equal_subtree(Ex::iterator i1, Ex::iterator i2,
300305
useprops_t use_props, bool ignore_parent_rel)
301306
{
302-
DEBUG( std::cerr << "equal_subtree with use_props = " << use_props << std::endl; );
307+
DEBUGLN( std::cerr << tab() << "equal_subtree with use_props = " << use_props << std::endl; );
303308
++offset;
304309

305310
Ex::sibling_iterator i1end(i1);
@@ -330,7 +335,7 @@ namespace cadabra {
330335
}
331336
bool ip = ignore_parent_rel && (dist==0);
332337
match_t mm=compare(i1, i2, first_call, up, ip);
333-
// DEBUG( std::cerr << "COMPARE " << *i1->name << ", " << *i2->name << " = " << static_cast<int>(mm) << std::endl; )
338+
// DEBUGLN( std::cerr << "COMPARE " << *i1->name << ", " << *i2->name << " = " << static_cast<int>(mm) << std::endl; )
334339
first_call=false;
335340
switch(mm) {
336341
case match_t::no_match_indexpos_less:
@@ -402,11 +407,11 @@ namespace cadabra {
402407
break;
403408
}
404409
// Continue walking the tree downwards.
405-
DEBUG( std::cerr << "one down" << std::endl; );
410+
DEBUGLN( std::cerr << tab() << "one down" << std::endl; );
406411
++i1;
407412
++i2;
408413
}
409-
DEBUG( std::cerr << "equal_subtree done" << std::endl; );
414+
DEBUGLN( std::cerr << tab() << "equal_subtree done" << std::endl; );
410415

411416
return report(worst_mismatch);
412417
}
@@ -431,7 +436,9 @@ namespace cadabra {
431436

432437
Ex_comparator::match_t Ex_comparator::report(Ex_comparator::match_t r) const
433438
{
439+
#ifndef DEBUG
434440
return r;
441+
#endif
435442

436443
std::cerr << tab() << "result = ";
437444
switch(r) {
@@ -475,7 +482,7 @@ namespace cadabra {
475482
// nobrackets also implies 'no multiplier', i.e. 'toplevel'.
476483
// 'one' is the substitute pattern, 'two' the expression under consideration.
477484

478-
DEBUG( std::cerr << tab() << "matching " << Ex(one) << tab() << "to " << Ex(two) << tab() << "using props = " << use_props << ", ignore_parent_rel = " << ignore_parent_rel << std::endl; );
485+
DEBUGLN( std::cerr << tab() << "matching " << Ex(one) << tab() << "to " << Ex(two) << tab() << "using props = " << use_props << ", ignore_parent_rel = " << ignore_parent_rel << std::endl; );
479486

480487
if(nobrackets==false && one->fl.bracket != two->fl.bracket)
481488
return report( (one->fl.bracket < two->fl.bracket)?match_t::no_match_less:match_t::no_match_greater );
@@ -501,9 +508,9 @@ namespace cadabra {
501508
// Things in _{..} or ^{..} are either indices (implicit patterns) or coordinates.
502509
const Coordinate *cdn1=0;
503510
if(use_props==useprops_t::always) {
504-
DEBUG( std::cerr << tab() << "is " << *one->name << " a coordinate?" << std::endl; );
511+
DEBUGLN( std::cerr << tab() << "is " << *one->name << " a coordinate?" << std::endl; );
505512
cdn1=properties.get<Coordinate>(one, true);
506-
DEBUG( std::cerr << tab() << cdn1 << std::endl; );
513+
DEBUGLN( std::cerr << tab() << cdn1 << std::endl; );
507514
}
508515

509516
if(cdn1==0)
@@ -532,9 +539,9 @@ namespace cadabra {
532539
//std::cerr << "**** can one take value two " << use_props << std::endl;
533540
const Integer *ip = 0;
534541
if(use_props==useprops_t::always) {
535-
DEBUG( std::cerr << tab() << "is " << *one->name << " an integer?" << std::endl; );
542+
DEBUGLN( std::cerr << tab() << "is " << *one->name << " an integer?" << std::endl; );
536543
ip = properties.get<Integer>(one, true); // 'true' to ignore parent rel.
537-
DEBUG( std::cerr << tab() << ip << std::endl; );
544+
DEBUGLN( std::cerr << tab() << ip << std::endl; );
538545
}
539546

540547
if(ip==0) return report(match_t::no_match_less);
@@ -575,7 +582,7 @@ namespace cadabra {
575582
// this node is an Accent, because then the child nodes are
576583
// very relevant).
577584
if(loc == replacement_map.end() && Ex::number_of_children(one)!=0) {
578-
DEBUG( std::cerr << tab() << "**** not found, trying without child nodes" << std::endl; );
585+
DEBUGLN( std::cerr << tab() << "**** not found, trying without child nodes" << std::endl; );
579586
if(properties.get<Accent>(one)==0 ) {
580587
Ex tmp1(one);
581588
tmp1.erase_children(tmp1.begin());
@@ -599,7 +606,7 @@ namespace cadabra {
599606
tmp2.erase_children(tmp2.begin());
600607
cmp=subtree_compare(&properties, (*loc).second.begin(), tmp2.begin(), -2);
601608
}
602-
DEBUG(std::cerr << " pattern " << two
609+
DEBUGLN(std::cerr << " pattern " << two
603610
<< " should be " << (*loc).second.begin()
604611
<< " because that's what " << one
605612
<< " was set to previously; result " << cmp << std::endl; );
@@ -618,18 +625,18 @@ namespace cadabra {
618625
return report(match_t::no_match_indexpos_greater);
619626

620627
if(one->is_index()) {
621-
DEBUG( std::cerr << tab() << "object one is index" << std::endl; );
628+
DEBUGLN( std::cerr << tab() << "object one is index" << std::endl; );
622629

623630
const Indices *t1=0;
624631
const Indices *t2=0;
625632
if(use_props==useprops_t::always) {
626-
DEBUG( std::cerr << tab() << "is " << one << " an index?" << std::endl; );
633+
DEBUGLN( std::cerr << tab() << "is " << one << " an index?" << std::endl; );
627634
t1=properties.get<Indices>(one, false);
628-
DEBUG( std::cerr << tab() << "found for one: " << t1 << std::endl; );
635+
DEBUGLN( std::cerr << tab() << "found for one: " << t1 << std::endl; );
629636
if(two->is_rational()==false) {
630-
DEBUG( std::cerr << tab() << "is " << two << " an index?" << std::endl; );
637+
DEBUGLN( std::cerr << tab() << "is " << two << " an index?" << std::endl; );
631638
t2=properties.get<Indices>(two, false);
632-
DEBUG( std::cerr << tab() << t2 << std::endl; );
639+
DEBUGLN( std::cerr << tab() << t2 << std::endl; );
633640
// It is still possible that t2 is a Coordinate and
634641
// t1 an Index which can take the value of the
635642
// coordinate. This happens when 'm' is an index
@@ -652,7 +659,7 @@ namespace cadabra {
652659
}
653660
else {
654661
t2=t1; // We already know 'one' can take the value 'two', so in a sense t2 is in the same set as t1.
655-
DEBUG( std::cerr << tab() << two << " is rational" << std::endl; );
662+
DEBUGLN( std::cerr << tab() << two << " is rational" << std::endl; );
656663
}
657664
}
658665

@@ -666,7 +673,7 @@ namespace cadabra {
666673
if(!ignore_parent_rel)
667674
if(t1==0 || t2==0 || (t1->position_type!=Indices::free && t2->position_type!=Indices::free))
668675
if(one->fl.parent_rel != two->fl.parent_rel) {
669-
DEBUG( std::cerr << tab() << "parent_rels not the same" << std::endl;);
676+
DEBUGLN( std::cerr << tab() << "parent_rels not the same" << std::endl;);
670677
return report( (one->fl.parent_rel < two->fl.parent_rel)
671678
?match_t::no_match_indexpos_less:match_t::no_match_indexpos_greater );
672679
}
@@ -701,7 +708,7 @@ namespace cadabra {
701708
// if we want that a found _{z} also leads to a replacement for ^{z},
702709
// this needs to be added to the replacement map explicitly.
703710

704-
DEBUG( std::cerr << "adding " << one << " -> " << two << " to replacement map " << std::endl; );
711+
DEBUGLN( std::cerr << "adding " << one << " -> " << two << " to replacement map " << std::endl; );
705712
replacement_map[one]=two;
706713

707714
// if this is an index, also store the pattern with the parent_rel flipped
@@ -739,7 +746,7 @@ namespace cadabra {
739746
replacement_map[tmp1]=tmp2;
740747
}
741748

742-
DEBUG( std::cerr << "Replacement map is now:" << std::endl;
749+
DEBUGLN( std::cerr << "Replacement map is now:" << std::endl;
743750
for(auto& rule: replacement_map)
744751
std::cerr << "* " << rule.first << " -> " << rule.second << std::endl;
745752
);
@@ -775,9 +782,9 @@ namespace cadabra {
775782
else if(is_coordinate || is_number) { // Check if the coordinate can come from an index.
776783
const Indices *t2=0;
777784
if(use_props==useprops_t::always) {
778-
DEBUG( std::cerr << tab() << "is " << *two->name << " an index?" << std::endl; );
785+
DEBUGLN( std::cerr << tab() << "is " << *two->name << " an index?" << std::endl; );
779786
t2=properties.get<Indices>(two, true);
780-
DEBUG( std::cerr << tab() << t2 << std::endl; );
787+
DEBUGLN( std::cerr << tab() << t2 << std::endl; );
781788
}
782789

783790
if(value_matches_index && t2) {
@@ -788,7 +795,7 @@ namespace cadabra {
788795
if(!ignore_parent_rel)
789796
if(t2->position_type==Indices::fixed || t2->position_type==Indices::independent) {
790797
if( one->fl.parent_rel != two->fl.parent_rel ) {
791-
DEBUG( std::cerr << tab() << "parent_rels not the same" << std::endl;);
798+
DEBUGLN( std::cerr << tab() << "parent_rels not the same" << std::endl;);
792799
return report( (one->fl.parent_rel < two->fl.parent_rel)
793800
?match_t::no_match_indexpos_less:match_t::no_match_indexpos_greater );
794801
}
@@ -927,7 +934,7 @@ namespace cadabra {
927934

928935
int sign=1;
929936
if(factor_locations.size()>0) {
930-
DEBUG(std::cerr << "--- can move? ---" << std::endl; );
937+
DEBUGLN(std::cerr << "--- can move? ---" << std::endl; );
931938
// Determining the sign is non-trivial to do step-by step.
932939
// Consider an expression
933940
// A B C D E
@@ -938,7 +945,7 @@ namespace cadabra {
938945
// no sign.
939946
Ex_comparator comparator(properties);
940947
sign=comparator.can_move_adjacent(st, factor_locations, start);
941-
DEBUG(std::cerr << "--- done can move ---" << sign << std::endl; );
948+
DEBUGLN(std::cerr << "--- done can move ---" << sign << std::endl; );
942949
}
943950
if(sign==0) { // object found, but we cannot move it in the right order
944951
replacement_map=backup_replacements;
@@ -1278,13 +1285,21 @@ namespace cadabra {
12781285
const SortOrder *so2=properties.get<SortOrder>(two,num2);
12791286

12801287
if(so1==0 || so2==0 || so1!=so2) {
1281-
// std::cerr << "No sort order between " << Ex(one) << " and " << Ex(two);
1288+
DEBUGLN( std::cerr << "No sort order between " << Ex(one) << " and " << Ex(two); );
12821289
report(subtree_comparison);
1283-
// std::cerr << std::endl;
1284-
// No explicit sort order known; use alpha sort.
1285-
if(subtree_comparison==match_t::subtree_match) return false;
1286-
if(subtree_comparison==match_t::no_match_less) return false;
1287-
if(subtree_comparison==match_t::no_match_greater) return true;
1290+
// No explicit sort order known; use whatever we know from the comparison.
1291+
switch(subtree_comparison) {
1292+
case match_t::node_match:
1293+
case match_t::subtree_match:
1294+
case match_t::no_match_less:
1295+
case match_t::no_match_indexpos_less:
1296+
case match_t::match_index_less:
1297+
return false;
1298+
case match_t::no_match_greater:
1299+
case match_t::no_match_indexpos_greater:
1300+
case match_t::match_index_greater:
1301+
return true;
1302+
}
12881303
return false;
12891304
}
12901305

‎tests/canonicalise.cdb‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -852,4 +852,14 @@ def test57():
852852
print("Test 57 passed")
853853

854854
test57()
855-
855+
856+
def test58():
857+
# https://cadabra.science/qa/2291/sort_product-with-position-independent-identical-variables
858+
{a,b,c,d,e,f,g}::Indices(vector,position=independent).
859+
ex := x_{a} x^{b} - x^{b} x_{a};
860+
sort_product(_);
861+
assert(ex==0)
862+
print("Test 58 passed")
863+
864+
test58()
865+

0 commit comments

Comments
 (0)