Latex variable name processing #38
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ZedThree
reviewed
Nov 20, 2024
ZedThree
reviewed
Nov 20, 2024
tests/test_basic.py
Outdated
| assert df["Current_Jz"].attrs["long_name"] == "Current J$_z$" | ||
| assert df["Particles_Px_Electron"].attrs["long_name"] == "Particles P$_x$ Electron" | ||
| assert df["Particles_Py_Electron"].attrs["long_name"] == "Particles P$_y$ Electron" | ||
| assert df["Particles_Pz_Electron"].attrs["long_name"] == "Particles P$_z$ Electron" |
There was a problem hiding this comment.
Probably also wise to have a couple of examples where we don't want this to happen
There was a problem hiding this comment.
Added new tests for this in 14a4b65
ZedThree
reviewed
Nov 20, 2024
Comment on lines
+137
to
+138
| assert _process_latex_name("Example") == "Example" | ||
| assert _process_latex_name("PxTest") == "PxTest" |
There was a problem hiding this comment.
I think it would be better to check the long name for one of the variables in df that we've not LaTeXified
There was a problem hiding this comment.
Decided to leave those as there aren't any examples of names like that but also validated against existing variables 9075383
ZedThree
reviewed
Nov 21, 2024
src/sdf_xarray/__init__.py
Outdated
Comment on lines
43
to
45
| affix_pattern = rf"(\s+){prefix}{suffix}(\s*|$)" | ||
| # Insert LaTeX format while preserving spaces | ||
| replacement = rf"\1${prefix}_{suffix}$\2" |
There was a problem hiding this comment.
I think this can just be this?
Suggested change
| affix_pattern = rf"(\s+){prefix}{suffix}(\s*|$)" | |
| # Insert LaTeX format while preserving spaces | |
| replacement = rf"\1${prefix}_{suffix}$\2" | |
| affix_pattern = rf"\b{prefix}{suffix}\b" | |
| # Insert LaTeX format while preserving spaces | |
| replacement = rf"${prefix}_{suffix}$" |
There was a problem hiding this comment.
Indeed it can! Ran it locally and checked tests passed
ZedThree
approved these changes
Nov 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change all
long_nameattributes to use spaces instead of underscores with Title CaseAdd LaTeX detection of variables that contain the following prefixes and suffixes:
It will convert from
Electric Field ExtoElectric Field E$_x$It will only convert to latex if there are spaces before and after OR if there is a space before. This is to ensure that we don't rename variables that might contain those characters within another word. e.g.
Exponential ...Fixes #36