Refactor text to use virtualfile_from_vectors instead of pandas tempfile#559
Refactor text to use virtualfile_from_vectors instead of pandas tempfile#559
Conversation
Modified virtualfile_from_vectors to use put_strings on last column instead of put_vectors if it has a string data type. In theory this should work for `text`, but in reality, a segmentation fault happens for GMT < 6.1.1.
|
This PR should work after we bump 6.1.1. Please update this branch and see if it works. |
|
This PR is already a good improvement (avoid using temporary files), although it doesn't fix #483. I think we can remove the |
|
Ok, I'll update the branch. |
| with Session() as lib: | ||
| file_context = dummy_context(textfiles) if kind == "file" else "" | ||
| if kind == "vectors": | ||
| if position is not None: | ||
| file_context = dummy_context("") | ||
| else: | ||
| file_context = lib.virtualfile_from_vectors( | ||
| np.atleast_1d(x), np.atleast_1d(y), np.atleast_1d(text) | ||
| ) |
There was a problem hiding this comment.
Is it more readable to rewrite the codes to:
if kind == "file":
...
elif kind == "vectors":
...
else:
...There was a problem hiding this comment.
Yes it would be more readable. I think the only reason I wrote it that way was because pylint complained about too many if-statements:
R0912: Too many branches (13/12) (too-many-branches)If readability is more important, I can increase the branch limit on our .pylintrc file, or I can try refactoring more of the code in text to reduce the number of if-statements. OR, we can just merge this in now and worry about it in the next PR that will fix #483.
There was a problem hiding this comment.
OR, we can just merge this in now and worry about it in the next PR that will fix #483.
OK to me.
Description of proposed changes
Non user-facing refactor to avoid the use of temporary files in the original
textimplementation at #321.Enhance theNote: below code will not work yet.textimplementation by allowing a list/array of angle/font/justify inputs. Something like so:Note that this requires GMT 6.1.1, as mentioned in #520, so it won't work yet.It works now.One step towards fixing issue at #483
Reminders
make formatandmake checkto make sure the code follows the style guide.doc/api/index.rst.