PDL::IO::Dumper frestore doesn't resolve duplicated ndarrays

[d-lamb]

If one uses fdump to save a data structure that has identical ndarrays (or possibly other objects), then fdump will recognize this and save space in the output file by having the "first" ndarray that was written be referenced by the following ndarrays. This is great, but it presents a problem when using frestore, I think because it triggers "my variable %s masks earlier declaration in same scope" warnings. Simple example script attached.
dumper_collision.pdl.txt

[mohawk2]

I attach here for others' visibility the script, which for some reason Derek chose to attach in a harder to read format:

use strict;
use warnings;
use PDL;
use PDL::IO::Dumper;
use constant OUTFILE=>'dumper_collision.pdld';

my $x = xvals(10);
my $y1 = $x;
my $y2 = 2*$x;
my $y3 = $x*$x;

my %plots = ('x1'=>$x, 'y1'=>$y1,
	     'x2'=>$x, 'y2'=>$y2,
	     'x3'=>$x, 'y3'=>$y3);

fdump(\%plots,OUTFILE);

my %restored = %{frestore(OUTFILE)};

print "$_: " . ($restored{$_} // '') . "\n" foreach (sort keys %restored);

[mohawk2]

@d-lamb Do you have any insights into what's going on? I think everyone knows how powerful your insight is into making PDL better :-)

[shawnlaffan]

Just an eyeball check, but it would seem the code in PDL::IO::Dumper::find_PDLs needs to track which ones it has already generated the my($PDL_... code for.

pdl/Basic/lib/PDL/IO/Dumper.pm

Lines 509 to 523 in 0874f26

	} elsif(UNIVERSAL::isa($_,'PDL')) {
	# In addition to straight PDLs,
	# this gets subclasses of PDL, but NOT magic-hash subclasses of
	# PDL (because they'd be gotten by the previous clause).
	# So if you subclass PDL but your actual data structure is still
	# just a straight PDL (and not a hash with PDL field), you end up here.
	#
	my($pdlid) = sprintf('PDL_%u',$$_);
	my(@strings) = &PDL::IO::Dumper::dump_PDL($_,$pdlid);
	$out .= $strings[0];
	$$sp =~ s/\$$pdlid/$strings[1]/g if(defined($strings[1]));
	}

Although it might be simpler to filter duplicates from the string before returning.

pdl/Basic/lib/PDL/IO/Dumper.pm

Line 532 in 0874f26

return $out;

[shawnlaffan]

The patch below deduplicates the variable generation at the top of the Data::Dumper string. A more efficient variant would use the my($PDL_94709587573920) parts of the line, and perhaps build an array internally, but the current approach works as a proof of concept given the my variable warnings no longer occur.

The remaining issue, however, is that the returned data structure still does not populate the x2, x3 or y1 hash values. This looks be a Data::Dumper issue, though, as I can reproduce without using PDL (see last code block below). I also don't imagine it to be a new issue.

diff --git "a/Basic/lib/PDL/IO/Dumper.pm" "b/Basic/lib/PDL/IO/Dumper.pm"
index 97f25c83..fe78c8cf 100644
--- "a/Basic/lib/PDL/IO/Dumper.pm"
+++ "b/Basic/lib/PDL/IO/Dumper.pm"
@@ -529,6 +529,12 @@ sub PDL::IO::Dumper::find_PDLs {
     }
   
   }
+
+  #  deduplicate
+  my %seen;
+  $out = join "\n", grep {!$seen{$_}++} split ("\n", $out);
+  $out .= "\n";
+
   return $out;
 }
 

####################
## PDL::IO::Dumper dump file -- eval this in perl/PDL.

{my($VAR1);
my($PDL_94625084675312) = (double(0,1,2,3,4,5,6,7,8,9));
my($PDL_94625084869520) = (double(0,2,4,6,8,10,12,14,16,18));
my($PDL_94625078249408) = (double(0,1,4,9,16,25,36,49,64,81));


$VAR1 = {
          'x1' => $PDL_94625084675312,
          'x2' => $VAR1->{'x1'},
          'x3' => $VAR1->{'x1'},
          'y1' => $VAR1->{'x1'},
          'y2' => $PDL_94625084869520,
          'y3' => $PDL_94625078249408
        };

}

Data::Dumper example:

perl -MData::Dumper -E'my $v = \1; my $x = {a => $v, b => $v}; my $y = Dumper $x; say $y; my $z = eval $y; say Dumper $z'
$VAR1 = {
          'a' => \1,
          'b' => $VAR1->{'a'}
        };

$VAR1 = {
          'a' => \1,
          'b' => undef
        };

[shawnlaffan]

And to address my own comment about self-referential structures, $Data::Dumper::Purity needs to be set to a true value.

[shawnlaffan]

Updated patch below.

It would probably also be sensible to locally set $Data::Dumper::Varname to VAR in case users have globally overridden it. It is an otherwise latent bug, although $VAR1 has been hard coded for many years.

diff --git "a/Basic/lib/PDL/IO/Dumper.pm" "b/Basic/lib/PDL/IO/Dumper.pm"
index 97f25c83..32b81989 100644
--- "a/Basic/lib/PDL/IO/Dumper.pm"
+++ "b/Basic/lib/PDL/IO/Dumper.pm"
@@ -95,6 +95,7 @@ convenience routine exists to use it.
 
 sub PDL::IO::Dumper::sdump {
 # Make an initial dump...
+  local $Data::Dumper::Purity = 1;
   my($s) = Data::Dumper->Dump([@_]);
   my(%pdls);
 # Find the bless(...,'PDL') lines
@@ -114,7 +115,7 @@ sub PDL::IO::Dumper::sdump {
   # find_PDLs call (which modifies $s using the s/// operator).
 
   my($s2) =  "{my(\$VAR1);\n".&PDL::IO::Dumper::find_PDLs(\$s,@_)."\n\n";
-  return $s2.$s."\n}";
+  return $s2.$s."\n\$VAR1}";
 
 #
 }
@@ -529,6 +530,12 @@ sub PDL::IO::Dumper::find_PDLs {
     }
   
   }
+
+  #  deduplicate
+  my %seen;
+  $out = join "\n", grep {!$seen{$_}++} split ("\n", $out);
+  $out .= "\n";
+
   return $out;
 }
 

[mohawk2]

Great work @shawnlaffan! Could you make a PR, with (critically) a test that would fail without your patch?

[shawnlaffan]

In progress work is on this branch on my fork: https://github.com/shawnlaffan/pdl/tree/io_dumper_dups

It looks like I need to rework more of the code as larger uuencoded ndarrays are not handled by the above approach.