Closed simonguichandut closed 4 months ago
Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!
Thanks for doing this, @simonguichandut ! It's tough for us to anticipate all the different representations, so making sure we're up to date on how codes represent molecules and aligning that with your expectations is really important. Thank you!
I tested this with a Castro and MAESTROeX plotfile and it works.
pre-commit.ci autofix
I'm going to follow through my comment from last week now. First, I'll rebase the branch to resolve merge conflicts.
Here goes nothing
I ended up radically re-orienting the PR's implementation. Hopefully the behavior I implemented is suitable.
Ok, CI is green now, so let's open back for review. @simonguichandut what do you think ?
The TeX formatting is a bit wonky now: molecules and descriptive names are missing the "X" that the isotopes have, and omega dot for isotopes is rendered like \dot{\omega}[X(He4)]
. I think it would be better to have Substance.to_tex()
return just the rendered name (e.g. ^{4}He
and C_{12}H_{24}
) and have setup_fluid_fields()
add the parentheses and X
/\dot{\omega}
.
Good points ! Separation of concerns FTW ! better now ?
Yes, much better. Can we also add the tex formatting to CastroFieldInfo?
done ! I'll rebase the branch again if you're happy with it
thanks @yut23 for your input ! Now I think we should give @simonguichandut a couple weeks to comments before we can merge.
Looks great to me! I've tested on MAESTROeX plotfiles, with and without molecules, and it works as expected.
Awesome, I squashed all my commits together, let me know if I should do the same with yours !
Awesome, I squashed all my commits together, let me know if I should do the same with yours !
I'm not sure what this means/does, but either way I will delete my branch and sync with the main repo! :)
I'm proposing to rewrite the branch history so it contains just one commit. With some tools (mainly git bisect
) makes it more confortable to debug if we need to revisit this change in the future. The reason I asked is that I don't want to make your life harder in any way, but if it sounds like it wouldn't, so I'll just go ahead.
How does this handle something like "H2", with is either an isotope (deuterium) or a molecule (molecular H)? We probably want to default to isotopes.
How does this handle something like "H2", with is either an isotope (deuterium) or a molecule (molecular H)? We probably want to default to isotopes.
H2 does get recognized as the isotope
Yes, that's intentional, because from what I understand molecules are rarely seen in this context, but it's a shame that there's no way to tell for sure from just looking at the string "H2"
.
@zingale I could solidify the current behavior by adding a test case for it if you'd like.
no, I think it's fine. Just wanted to double check.
Alright let's just merge then. Thanks all !
Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! :fireworks:
PR Summary
The boxlib dataset parsing (for CASTRO/MAESTROeX) are currently set up for usual atomic species like "He4" or "C12", or descriptive names like "ash" with no numbers. The parsing breaks for e.g. molecules like "H2O" or "C6H6". This fix escapes those cases.
Ideally, we could have some fancy regex to correctly parse the molecules (and create proper a TeX string), but this is a very niche use of those codes.
PR Checklist