Opened 3 years ago

Last modified 3 months ago

#27025 needs_work defect

NULL and multiple output from R

Reported by: novoselt Owned by:
Priority: major Milestone: sage-9.7
Component: interfaces Keywords:
Cc: gh-timokau, fbissey Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: u/gh-timokau/r-empty-results (Commits, GitHub, GitLab) Commit: f05ee1203de8eeb6daa67cb8770822f9ab144b82
Dependencies: Stopgaps:

Status badges

Description

In Sage 8.4 we had

sage: r.eval("graphics.off()")
''
sage: r.eval("""
....: 1
....: 2
....: """)
'[1] 1\n[1] 2'

in Sage 8.5 this gives

sage: r.eval("graphics.off()")
'NULL'
sage: r.eval("""
....: 1
....: 2
....: """)
'[1] 2'

which is annoying and inconsistent with the output one gets from R using these commands:

> graphics.off()
> 1
[1] 1
> 2
[1] 2

I've also seen this example in the documentation:

sage: r.png(filename='"%s"'%filename)             # optional -- rgraphics
NULL

which again I think is wrong - there should be no NULL output when saving a file.

Change History (27)

comment:1 Changed 3 years ago by novoselt

As an example of why this NULL is annoying: http://faculty.sfasu.edu/judsontw/math-s304/section-16.html

Also, if there were cells relying on multiple output from multiple commands, they behave differently now as well.

comment:2 Changed 3 years ago by embray

I agree this should definitely be fixed.

Incidentally, this is the opposite of what I did in the recent updates to the libgap interface (#22626): Previously, void-returning GAP functions would have their "return value" wrapped in a null GapElement that actually repr'd as NULL which was obviously annoying and distracting.

In this case, the old behavior still returned an empty string, which still looks wrong to me. It should check whether the expression in question has an actual return value or not, and if not then r.eval should just return None.

comment:3 Changed 3 years ago by gh-timokau

Right, thanks for reporting. The eval function actually just hands of to rpy2:

def eval(self, code, *args, **kwds):
    self._lazy_init()
    return str(robjects.r(code)).rstrip()

In this case robjects.r("graphics.off()") is rpy2.rinterface.NULL, which unfortunately is implemented to have a string representation of NULL. I'd argue this is an upstream bug bug since I think upstream doesn't want to do new python2 releases, we need to fix it ourselves here.

The fix should be trivial. Just add a if result == rpy2.rinterface.NULL to eval. However do you know why

sage: r.png(filename='"%s"'%filename)             # optional -- rgraphics
NULL

is different? I didn't touch that in my recent R changes, so that always returned NULL.

comment:4 Changed 3 years ago by gh-timokau

  • Branch set to u/gh-timokau/r-empty-results
  • Commit set to f05ee1203de8eeb6daa67cb8770822f9ab144b82
  • Status changed from new to needs_review

Tested only the r.py file without any optionals though.


New commits:

f05ee12Fix R interface string representation of empty results

comment:5 Changed 3 years ago by novoselt

No idea about r.png returning NULL before and after - I thought it was a recent change.

But apart from NULL there is also change in behaviour for multi-line input. For SageMathCell, that adds that extra graphics command, this means that default output of single commands is not shown at all: https://sagecell.sagemath.org/?z=eJwzBAAAMgAy&lang=r Is it easy to restore old behaviour for this case?

comment:6 Changed 3 years ago by gh-timokau

I'm not entirely sure what you mean. Do you mean multiline input to eval()? How should that behave? Just pass each line to R separately? Does R have some sort of line-continuation that must be considered?

comment:7 Changed 3 years ago by novoselt

I gave an example in the description of how things worked before and what happens now. I don't know how it should be implemented as my knowledge of R is extremely limited, I only learned enough to save graphics to a file ;-)

comment:8 Changed 3 years ago by gh-timokau

For your examples just splitting on newline would work. But the same issue seems to appear any time you eval more than one statement, for example r.eval("1;3").

I'd personally argue that is out of scope for eval, but since it was supported before...

I don't know much about R myself, I only messed with it in the first place because it was stuck on an old version for so long and nobody else was doing anything.

comment:9 follow-up: Changed 3 years ago by embray

In

         """
         self._lazy_init()
-        return str(robjects.r(code)).rstrip()
+        result = robjects.r(code)
+        if result == rinterface.NULL:
+            # trac #27025
+            result = None
+        else:
+            result = str(result)
+
+        if isinstance(result, str):
+            # no trailing newlines
+            result = result.rstrip()
 
+        return result

This looks like it could be simplified: If result != NULL then you always do result = str(result) anyways, so why not just move the .rstrip() into the statement in the else: block and get rid of the superfluous isinstance(result, str)?

I'm also surprised now, upon reading the docs for the eval method, that it always returns the result s a string, and not the actual wrapped R object itself (as the libgap interface would do). Why is this? That makes me rethink having it ever return None in the first place. Though I'd be more inclined to say the eval-returns-a-string interface is a mistake in the first place and should be done away with.

comment:10 in reply to: ↑ 9 Changed 3 years ago by gh-timokau

Replying to embray:

In

         """
         self._lazy_init()
-        return str(robjects.r(code)).rstrip()
+        result = robjects.r(code)
+        if result == rinterface.NULL:
+            # trac #27025
+            result = None
+        else:
+            result = str(result)
+
+        if isinstance(result, str):
+            # no trailing newlines
+            result = result.rstrip()
 
+        return result

This looks like it could be simplified: If result != NULL then you always do result = str(result) anyways, so why not just move the .rstrip() into the statement in the else: block and get rid of the superfluous isinstance(result, str)?

I think I had a reason for that, but I can't remember it and I can't see any reason now. So it can probably be changed.

I'm also surprised now, upon reading the docs for the eval method, that it always returns the result s a string, and not the actual wrapped R object itself (as the libgap interface would do). Why is this? That makes me rethink having it ever return None in the first place. Though I'd be more inclined to say the eval-returns-a-string interface is a mistake in the first place and should be done away with.

If you wanted a proper object, you'd use __call__ instead: r("42"). Not sure why its done that way, I assume its just a symptom of having been a pexpect interface before. There it was natural to eval first and then translate to an object. Now we do it the other way around: get an object first, then produce a string if requested.

Gap does the same thing:

sage: type(gap.eval('42;'))
<type: 'str'>

comment:11 follow-up: Changed 3 years ago by embray

Right, I believe we talked about that before on the mailing list. I understand that the existing interface makes sense in light of its origin as a pexpect interface. Perhaps this should in fact be moved into sage.lib instead of sage.interfaces and updated to a more common interface, while deprecating the old interface. Now that I think about it I think it should have been done that way in the first place, but I didn't speak up in time so oh well. Could you please open a ticket for that? I think we should make a stronger effort to be consistent.

comment:12 Changed 3 years ago by embray

  • Milestone changed from sage-8.6 to sage-8.7

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

comment:13 in reply to: ↑ 11 Changed 3 years ago by gh-timokau

Replying to embray:

Right, I believe we talked about that before on the mailing list. I understand that the existing interface makes sense in light of its origin as a pexpect interface. Perhaps this should in fact be moved into sage.lib instead of sage.interfaces and updated to a more common interface, while deprecating the old interface. Now that I think about it I think it should have been done that way in the first place, but I didn't speak up in time so oh well. Could you please open a ticket for that? I think we should make a stronger effort to be consistent.

I don't think it should have been done like that. The point was to provide a drop-in replacement for the old interface. If we just want an R interface without backward compatibility concerns, I'd just tell users to use rpy2 directly.

comment:14 Changed 3 years ago by embray

Except it's clearly not just a drop-in replacement; it's a rather different way to go about things. Regardless, what's done is done, and it's still by far a net positive so I'm not complaining :)

comment:15 Changed 3 years ago by gh-timokau

It mostly is. Due to my little R usage and a lack of tests, some issues were probably inevitable. I'm only aware of this issue (the multiline thing may be hard to fix depending on how R handles these) and the multiple instances issue (wontfix from me).

I wouldn't be opposed to deprecating the old api altogether and using rpy2 (maybe a very thin wrapper) instead. But I don't want to fight that fight myself :)

comment:16 Changed 3 years ago by novoselt

  • Status changed from needs_review to needs_work

If someone is just interested in using R, then Sage is probably not the best way to do it. If someone just wants to use R from Sage/Python specifically, then rpy2 is quite sensible with whatever decisions it comes. But the point of particular output from eval etc. is that every interface in Sage (at least in theory), behaves this way. In particular, it is supposed to return a string and it used to return the empty string rather than None as "no output". I think this definitely has to be the case again.

Regarding multiline treatment - it may be less important as notebooks often show only the last output from commands in a single "cell" and a single call to eval is kind of like executing a "cell". It is still unfortunate that old behaviour is changed without deprecation period, but if nobody else complains and nobody wants to work on it, well then...

comment:17 Changed 3 years ago by embray

I will look into it at some point but it's definitely not a priority for me right now. As you said, while it's absolutely useful to have an R interface in Sage, that's useful more as a glue layer, and not for doing serious R programming, which should just be done in an R-specific UI.

comment:18 Changed 3 years ago by novoselt

Except, of course, for using it via SageMathCell where Sage is a necessary proxy between browser and R ;-) So I do hope that the issue will be solved in the next 8.7 release...

comment:19 Changed 3 years ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:20 Changed 3 years ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).

comment:21 Changed 3 years ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:22 Changed 2 years ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

comment:23 Changed 22 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:24 Changed 17 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:25 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

Setting a new milestone for this ticket based on a cursory review.

comment:26 Changed 7 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:27 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.6 to sage-9.7
Note: See TracTickets for help on using tickets.