bugPSPP - Bugs: bug #37999, RANK assert-fails with TEMPORARY

 
 

bug #37999: RANK assert-fails with TEMPORARY

Submitter:  Ben Pfaff <blp>
Submitted:  Sun 30 Dec 2012 04:56:17 AM UTC
   
 
Category:  Syntax Parser Severity:  5 - Average
Status:  Fixed Assigned to:  None
Open/Closed:  Closed Release:  Before 0.8.0
Effort:  0.00
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 02 Feb 2013 05:40:50 PM UTC, comment #9: 

Fixed on master with commit 97de936d2 (RANK: Add support for temporary transformations.).

Ben Pfaff <blp>
Group administrator
Fri 01 Feb 2013 06:04:30 AM UTC, comment #8: 

I posted a series of patches that fix the problem to pspp-dev.  They are ready for review.

Ben Pfaff <blp>
Group administrator
Thu 31 Jan 2013 07:09:45 AM UTC, comment #7: 

The bug37999 branch at git://benpfaff.org/pspp is getting pretty close.  I think it'll probably take about another hour of cleanup (tomorrow?), then I'll post it for review.

Ben Pfaff <blp>
Group administrator
Sat 26 Jan 2013 07:22:30 PM UTC, comment #6: 

The bug37999 branch at git://benpfaff.org/pspp appears to work with TEMPORARY on RANK, but it needs cleanup and more testing (including unit tests).

Ben Pfaff <blp>
Group administrator
Thu 24 Jan 2013 06:30:43 AM UTC, comment #5: 

I now have a rewritten RANK that passes all of the existing tests and should be much easier to convert to work with TEMPORARY.  It needs cleanup and some memory leak fixes before I can reasonably propose it for inclusion.

Ben Pfaff <blp>
Group administrator
Tue 22 Jan 2013 05:04:11 AM UTC, comment #4: 

I've made major progress on fixing this up.  It isn't ready to post yet, but I hope for that soon.

Ben Pfaff <blp>
Group administrator
Tue 08 Jan 2013 09:47:50 AM UTC, comment #3: 

There are several problems, which I think need to be dealt with separately (but perhaps concurrently).

1. TEMPORARY needs to create an internal variable, which records
   the original positions of the cases.

2. RANK currently is implemented in a rather funny way with regard
   to splits.  It performs its initial sort before considering
   splits.  This was done as an optimisation effort - if the split
   variable and the rank variable happen to be the same, then only
   one sort is required.   However this way of doing things makes
   the procedure hard to maintain.  I think we should remove this
   optimisation and tidy up a bit before trying to fix the problem.
   After the problem is fixed, we can consider optimisation.

3. RANK currently creates an "internal" variable (called
   $ORDER).   This   will no longer be required in the case of
   TEMPORARY, because it will have been done by [1].  But if
   there is no TEMPORARY in effect, we will still have to do it
   in RANK itself.  We need to make sure that in both cases, it
   is done consistently (ideally using the same code).

John Darrington <jmd>
Group administrator
Thu 03 Jan 2013 08:51:12 PM UTC, comment #2: 

jmd and I talked over approaches a bit in IRC today.  Here's the log:

jmd: blp: I think we might need some new functions to deal with temporary transformations to properly fix this RANK problem.
10:20
blp: jmd: Yes, that's why I've been looking at other things ;-)
blp: jmd: It's probably going to require nontrivial changes.
jmd: I've been quietly worring about something like this with RANK for a while now.
blp: I've been thinking of other ways to implement data anyhow.
blp: But those are probably inappropriate to fix this problem, since they require large whole-system changes.
blp: One kind-of-inefficient way to solve the problem would be to, at every use of TEMPORARY, introduce a new system variable that indexes cases.
jmd: Well I think we should try to find a stop-gap way of fixing the symptom, even if it's not particularly elegant.
blp: It's kind of inefficient since in most cases the variable won't be needed, but it should allow us to solve the problem.
blp: What do you think of that as a stopgap?
10:25
jmd: we could try it.
jmd: Isn't there already a $CASENUM system variable?
blp: $CASENUM isn't a real variable, the expressions code just synthesizes it.
blp: It doesn't count cases in a consistent way, either, in the sense that a SELECT IF before an expression will not cause $CASENUM to skip values.
blp: We'd want consistent case numbering at the point where we hit TEMPORARY, I think.

Ben Pfaff <blp>
Group administrator
Sun 30 Dec 2012 06:11:17 PM UTC, comment #1: 

Zoltan provided the following example, with the attached SPSS output:


DATA LIST TABLE /x 1 gender 3 (A).
BEGIN DATA.
1 f
2 m
3 f
4 m
5 f
6 m
7 f
8 m
9 f
end data.
FREQUENCIES x gender.
RANK x.
DISPLAY DICTIONARY.
TEMPORARY.
SELECT IF (gender="f").
RANK x /ntiles (2) into y1.
FREQUENCIES y1.

TEMPORARY.
SELECT IF (gender="m").
RANK x /ntiles (2) into y2.
FREQUENCIES y2.


(file #27167)

Ben Pfaff <blp>
Group administrator
Sun 30 Dec 2012 04:56:17 AM UTC, original submission:  

The following assert-fails:


data list list /x.
begin data.
1
2
3
end data.
TEMPORARY.
RANK x.


Reported by Zoltan Fabian.

Ben Pfaff <blp>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #27167:  spss_output.txt added by blp (24KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jmd (Posted a comment)
  • -email is unavailable- added by blp (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-02-02 blp StatusReady for Test/Review Fixed
        Open/ClosedOpen Closed
    2013-02-01 blp StatusConfirmed Ready for Test/Review
    2012-12-30 blp Attached File- Added spss_output.txt, #27167
    2012-12-30 blp ReleaseNone Before 0.8.0

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code