allow amrex substantance fields to have an "_"#5115
allow amrex substantance fields to have an "_"#5115zingale wants to merge 4 commits intoyt-project:mainfrom
Conversation
this allows for NSE protons in our output ("P_nse")
|
@zingale Do you mind if I fix those problems and add some tests? |
|
I'm happy with the changes |
|
nose test failure was from an aborted run after timing out:
probably a fluke (since other tests ran recently no problem), will trigger it again. |
|
@yt-fido test this please |
|
Is this one waiting on a review? |
|
yes. I think that since I started it and @yut23 then modified it, someone else should review. |
neutrinoceros
left a comment
neutrinoceros
left a comment
There was a problem hiding this comment.
Looks fine, and mostly I have questions more than demands, though there's one point I'd really want to see taken into account about making escaping idempotent.
|
|
||
| def _to_tex_descriptive(self) -> str: | ||
| return str(self) | ||
| return self._tex_escape(str(self)) |
There was a problem hiding this comment.
any reason not to update the corresponding __str__ implementation instead ?
|
|
||
| @staticmethod | ||
| def _tex_escape(s: str) -> str: | ||
| return s.replace("_", r"\_") |
There was a problem hiding this comment.
I can see this snowballing into a subtle bug where a single underscore would be escaped multiple times instead of once. I'm sure we make this method idempotent using a regexp:
| return s.replace("_", r"\_") | |
| # replace '_' with '\_' once, but leave already-escaped occurrences alone | |
| return re.sub(r"[^\\]_", r"\\_", s) |
There was a problem hiding this comment.
This regexp deletes the non-backslash character preceding an underscore. It should go in a capturing group, or just use a negative lookbehind instead
| pytest.param("X(C12H24)", [("C", 12), ("H", 24)], id="molecule_2"), | ||
| pytest.param("X(H2O)", [("H", 2), ("O", 1)], id="molecule_3"), | ||
| pytest.param("X(ash)", [("ash", 0)], id="descriptive_name"), | ||
| pytest.param("X(P_nse)", [("P_nse", 0)], id="descriptive_underscore"), |
There was a problem hiding this comment.
I did a double take here: the underscore isn't descriptive by itself, or is it ?
| pytest.param("X(P_nse)", [("P_nse", 0)], id="descriptive_underscore"), | |
| pytest.param("X(P_nse)", [("P_nse", 0)], id="with_an_underscore"), |
There was a problem hiding this comment.
(the same applies to the other two test cases)
There was a problem hiding this comment.
I don't understand the concerrn
|
I'm adding this to the 4.5.0 milestone as it's not currently labeled as a bugfix, but if you guys would like it backported to yt 4.4.x, this seems fine to me, so don't hesitate to changie the milestone accordingly and update the labels (enhancement -> bug). |
|
@zingale looks like there are some remaining comments for you to check out here. |
Co-authored-by: Clément Robert <cr52@protonmail.com>
4 participants
this allows for NSE protons in our output ("P_nse")
PR Summary
PR Checklist