Fix small bugs/edge cases in the parthenon frontend #5276
Conversation
|
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! |
| if len(ds.shape) == 4: | ||
| data = ds[start:end, :, :, :].transpose() | ||
| else: | ||
| data = ds[start:end, fdi, :, :, :].transpose() |
There was a problem hiding this comment.
Parthenon's python reader can always split a variable into a flat index space, so I think this is now generic.
There was a problem hiding this comment.
I'm not sure I understand this change. What's exactly happening here?
a) the Parthenon python reader is not used in yt
b) a flattened index space (from a vector or tensor variable) would always carry the fdi index, doesn't it?
There was a problem hiding this comment.
In riot's current format scalar fields are only 4D with the leading index the block. AthenaPK doesn't see this because it has a prim vector, but riot and phoebus do. That's what's this change is. I didn't modify the piece with prim vector, just special case scalars.
|
I believe this MR is a bugfix but I don't seem to have permissions to add a label. |
|
@Yurlungur do you have any data to share with those coordinate that could be added to the regression tests? |
Yes happy to provide something. In fact, for the scalar field check, even output from Parthenon's poisson example would do the trick. For cylindrical coordinates... I'm not sure we have anything, but I could mock something up. I can't access yt-hub, however, so I'd have to pass the file on to someone else to upload. |
|
@pgrete sent you a dataset. Can you shepherd it into the tests, or should I try to do so? |
|
Dataset PR is here: yt-project/website#141 Also, I added a skeleton on how to test the data (currently just checking for the field name). |
|
btw if you quickly want to run local tests, just where |
Sorry for the delay. Been traveling. I added a check that the cell width in theta is 2pi. (This was being incorrectly overwritten, by the way, which I fixed.) I also added a check for the total mass. |
|
@chrishavlin this now looks good to me. |
|
@pgrete I think we need @Xarthisius to move the new dataset from yt-project/website#141 to where it needs to go... but the test server that runs the full test suite is currently on the fritz, so the full nose test suite will not run at present (right now, the only tests that are running are those that do not require sample datasets). Doesn't hurt to have the dataset in place for when we revamp the test server, but not sure when that will be. In the meantime, I'll double check that the new tests run for me locally right now and we should be good to merge. |
chrishavlin
left a comment
chrishavlin
left a comment
There was a problem hiding this comment.
I did run the existing answer tests locally and confirmed that they passed -- was not able to run the new test due to an error in unpacking the new test file (see yt-project/website#141 (comment) ).
One final minor code suggestion if you want to fix it, but IMO this is good to merge. I'll hit approve once we get the new test file figured out.
chrishavlin
left a comment
chrishavlin
left a comment
There was a problem hiding this comment.
just ran the new test locally with the updated test file and it passed as expected, so this PR is good to go from my perspective - @Yurlungur , i did add one comment earlier today on an optional minor code suggestion (here: https://github.com/yt-project/yt/pull/5276/files#r2411258480 ), I'll give you a chance to make the change or let me know if you don't want to bother, then we can merge!
Co-authored-by: Chris Havlin <[email protected]>
Thanks, @chrishavlin sounds good. Suggestion merged! |
|
@Yurlungur -- you'll also have to remove the reference to in the superclass Oh, the pre-commit check actually picked it up: https://results.pre-commit.ci/run/github/88776493/1760031924.mgAcmADwSAq7-ms9whb8BA |
Oops... Sorry about that. I failed my reading comprehension test. Fixed. |
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
this is good to merge after tests pass again |
|
just merged with main, should take care of that one failing test and then we can merge! |
|
Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆 |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
…on frontend Merge pull request yt-project#5276 from Yurlungur/jmm/parthenon-frontend-updates Fix small bugs/edge cases in the parthenon frontend (cherry picked from commit e962538)
|
🎊 Thanks for your feedback and helping pull it all through @chrishavlin @pgrete ! |
…on-yt-4.4.x Backport PR #5276: Fix small bugs/edge cases in the parthenon frontend
5 participants
PR Summary
We recently finally refactored curvilinear coordinates in Parthenon and the ordering for cylindrical coordinates was normalized to the standard choice in yt. This MR fixes that in parthenon frontend. I also noticed that the yt frontend was not properly handling variables of tensor rank less than 1 (i.e., scalars on the mesh). This MR adds a one-line fix that gets that working.
PR Checklist