Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stop raster output from being resampled #55

Merged
merged 10 commits into from Feb 21, 2014
Merged

Conversation

smason
Copy link

@smason smason commented Jan 17, 2014

this is in reference to issue #54

@krlmlr
Copy link
Collaborator

krlmlr commented Jan 17, 2014

Thanks. Since the tests fail, I think we should make this an optional parameter for the tikz function. (Also to maintain compatibility.) Perhaps something like rasterResolution with the current default, and NA meaning unchanged resolution (=your implementation)? Please also add a test.

@krlmlr
Copy link
Collaborator

krlmlr commented Jan 17, 2014

Could you please also rebase to current master? You can force-push (git push -f), the pull request will update itself.

@smason
Copy link
Author

smason commented Jan 17, 2014

Hadn't thought of putting this under the "rasterResolution" flag; seems like the sensible option.
Will hack in the old code under this switch and update the request.

@smason
Copy link
Author

smason commented Jan 17, 2014

sorry about all those commits; I forgot a couple of issues and OS X and Linux appear to produce different PNGs which cause the tests to fail.

everything seems to be working now!

@krlmlr
Copy link
Collaborator

krlmlr commented Jan 17, 2014

Thanks, I'm fine with the commits. Could you please add documentation, and an item to NEWS.md? (I now see in the commits that you are Sam Mason, if you want you can change the entry I did as well.)


res = getOption('tikzRasterResolution')
if (is.null(res)) res = NA;
if (is.na(res)) interpolate = FALSE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

else if?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, now I see that else would be wrong. Sorry.

Copy link
Author

Choose a reason for hiding this comment

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

yes, maybe a comment explaining what I'm doing would help?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would make things clearer. On the other hand, does the setting of interpolate make a difference at all if res is NA?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it seems to make a difference. I get something very different from what I put in without this

@smason
Copy link
Author

smason commented Jan 23, 2014

@krlmlr, I've had a tidy up of the code based on your comments.
I've put some lines into the vignette and -package file, and a comment in the NEWS file.
Hope these are OK.

krlmlr added a commit that referenced this pull request Feb 21, 2014
stop raster output from being resampled
@krlmlr krlmlr merged commit 2483765 into daqana:master Feb 21, 2014
@krlmlr
Copy link
Collaborator

krlmlr commented Feb 21, 2014

Thanks!

tools::file_path_sans_ext(fileName),
'_ras', rasterCount, '.png')

res = getOption('tikzRasterResolution')
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can use res = getOption('tikzRasterResolution', NA) and delete the three lines below

Copy link
Author

Choose a reason for hiding this comment

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

hadn't noticed that parameter before—thanks! Have pushed an appropriate change to my noresample repo

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please open a new pull request?

Copy link
Author

Choose a reason for hiding this comment

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

sorry, I'm sure when I get people hacking my repos I'll appreciate all these ways of moving code around!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sure you will ;-) Merging with a few clicks is very comfortable indeed.

@krlmlr
Copy link
Collaborator

krlmlr commented Jan 21, 2016

I'm now revisiting this code due to reentrance problems (#132), my approach is to write the raster "as is" using png::writePNG(), therefore obliterating the tikzRasterResolution option. Given that PGF is in full control of the final raster's dimensions and orientation, setting a DPI value in the created PNG file doesn't seem to be useful anyway. The only thing that's lost is a rescaling of raster images, but I guess nobody cares about that anyway -- if at all, it should affect only the file size of the output.

@smason: Do you think this makes sense?

@smason
Copy link
Author

smason commented Jan 21, 2016

Yes, assuming I'm interpreting you correctly, it sounds reasonable.

Whenever I've sent a raster to output in R I've wanted it preserved exactly, and any resampling would be very undesirable. Further, I've not noticed any other R device I've used resampling any raster given to it.

@krlmlr
Copy link
Collaborator

krlmlr commented Jan 21, 2016

Thanks for your fast reply. I've noticed that R seems to be doing all the resampling it needs already: In particular, the base_raster test looks almost unchanged when resampling is turned off; this was a pleasant surprise for me.

@krlmlr
Copy link
Collaborator

krlmlr commented Jan 21, 2016

Wrong: It's done by PGF, via interpolate=true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants