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

RFC Allow for io.StringIO inputs to certain modules #576

Closed
wants to merge 4 commits into from

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Sep 2, 2020

Description of proposed changes

Make a tempfile_from_buffer backend function for io.StringIO inputs. Will be useful for modules where:

This is implemented as the 'hacky' Option 1 at #571 (comment).

Happy to discuss about the actual implementation details, such as:

  • whether 'tempfile_from_buffer' is a good name
  • should this live inside 'tempfile.py', or is it better placed with the other 'virtualfile_from_*' functions (which is what I modeled 'tempfile_from_buffer' after).
  • etc

See also:

Fixes #571

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

@weiji14 weiji14 added the enhancement Improving an existing feature label Sep 2, 2020
@weiji14 weiji14 changed the title Allow for io.StringIO inputs to certain modules RFC Allow for io.StringIO inputs to certain modules Sep 2, 2020
There is a new 'buffer' data_kind which is
intended for io.StringIO stream data.
This 'buffer' data is passed into `legend`
via an intermediate file created using the
helper tempfile_from_buffer function.
@weiji14 weiji14 mentioned this pull request Sep 6, 2020
11 tasks
@weiji14
Copy link
Member Author

weiji14 commented Sep 7, 2020

@dr-glenn, do you want to try this branch using pip install https://github.com/GenericMappingTools/pygmt/archive/stringio_input.zip and see if it works for your StringIO legend? Documentation (beta) is at https://pygmt-git-stringioinput.gmt.vercel.app/api/generated/pygmt.Figure.legend.html#pygmt.Figure.legend.

Happy to receive any feedback before we merge this. If it's good, we might get this in for the upcoming PyGMT v0.2.0 release 😁

@seisman
Copy link
Member

seisman commented Sep 7, 2020

io.StringIO is something new to me. Some googling gives the impression that io.StringIO is one of the file-like objects. Perhaps we should generalize the function to support all file-like objects?

@weiji14
Copy link
Member Author

weiji14 commented Sep 7, 2020

io.StringIO is something new to me. Some googling gives the impression that io.StringIO is one of the file-like objects. Perhaps we should generalize the function to support all file-like objects?

I'm a pretty new user of io.StringIO too. I'm aware of io.BytesIO which is a binary file-like object, but I'm not quite sure if it is commonly used and/or whether it's worth supporting. Maybe @dr-glenn knows more.

To be fair, we'll probably need to refactor this hacky implementation (writing to a tempfile and having GMT read that file) to use GMT_Read_Data or something else at some point. Not to say that this PR isn't useful, just that we can add these 'enhancements' in one step at a time.

@seisman
Copy link
Member

seisman commented Sep 7, 2020

To be fair, we'll probably need to refactor this hacky implementation (writing to a tempfile and having GMT read that file) to use GMT_Read_Data or something else at some point.

Yes, that's another point. Without this PR, users just need to add a few lines of codes (maybe two or three) to write strings into a temporary file, and pass the file name to legend(), text() or any other modules. But here you add several lines of codes to support it, and these codes are most likely to be removed in the future. Not sure if it's really worth the complexity.

@weiji14
Copy link
Member Author

weiji14 commented Sep 7, 2020

Yes, that's another point. Without this PR, users just need to add a few lines of codes (maybe two or three) to write strings into a temporary file, and pass the file name to legend(), text() or any other modules. But here you add several lines of codes to support it, and these codes are most likely to be removed in the future. Not sure if it's really worth the complexity.

Fair point, the code here is really just a convenience function to simplify maybe 4-5 lines of code:

with GMTTempFile() as tmpfile:
buf.seek(0) # Change stream position back to start
with open(file=tmpfile.name, mode="w") as fdst:
shutil.copyfileobj(fsrc=buf, fdst=fdst)
yield tmpfile.name

As for whether it's worth it, there are some GMT Supplemental Modules that don't quite support virtualfiles yet (e.g. x2sys, as mentioned in #546 (comment)), and having this tempfile_from_buffer helper function in the backend would be useful in the interim months to pass in pandas.DataFrame inputs to these modules.

@seisman
Copy link
Member

seisman commented Apr 12, 2022

@weiji14 I think it's time to refactor this PR, so that all functions can accept io.StringIO input.

@weiji14
Copy link
Member Author

weiji14 commented Apr 12, 2022

@weiji14 I think it's time to refactor this PR, so that all functions can accept io.StringIO input.

Ok, but we really should make the input types tutorial at #1268 a priority first. I feel that new users don't know all the I/O options PyGMT supports, like passing in a shapefile...

@weiji14 weiji14 added this to the 0.7.0 milestone Apr 12, 2022
@weiji14 weiji14 modified the milestones: 0.7.0, 0.9.0 Jun 15, 2022
@weiji14 weiji14 removed this from the 0.9.0 milestone Mar 6, 2023
@seisman
Copy link
Member

seisman commented Sep 13, 2024

This PR will be superseded by PR #3326 and #3438. Closing.

@seisman seisman closed this Sep 13, 2024
@seisman seisman deleted the stringio_input branch September 13, 2024 14:30
@seisman seisman added this to the 0.14.0 milestone Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Figure.legend to read from StringIO
2 participants